Skip to content

Conversation

@Abhirup-99
Copy link

@Abhirup-99 Abhirup-99 commented Aug 25, 2025

This pull request introduces support for streaming HTTP response bodies in the client, allowing responses to be read as streams rather than being fully loaded into memory. This is particularly useful for handling large responses or server-sent events. The changes include new configuration options at both the client and request levels, implementation of the streaming logic, and comprehensive tests to ensure correct behavior.

Streaming response body support:

  • Added a streamResponseBody field to the Client struct, along with SetStreamResponseBody and StreamResponseBody methods to enable or disable response body streaming at the client level (client/client.go). [1] [2]
  • Added a streamResponseBody field to the Request struct, with corresponding SetStreamResponseBody and StreamResponseBody methods to allow per-request configuration that overrides the client setting (client/request.go). [1] [2]
  • Updated the request execution logic to set and restore the underlying HTTP client's streaming option based on the request or client configuration (client/core.go).
  • Implemented the BodyStream method on the Response struct to provide an io.Reader for streaming the response body, falling back to an in-memory reader if streaming is not enabled (client/response.go).

Testing and validation:

  • Added extensive tests to verify streaming behavior, including tests for server-sent events, large responses, default and overridden settings, and chainable configuration methods (client/client_test.go, client/response_test.go). [1] [2]
  • Ensured that request resetting clears the streamResponseBody override (client/request.go).# Description

Please provide a clear and concise description of the changes you've made and the problem they address. Include the purpose of the change, any relevant issues it solves, and the benefits it brings to the project. If this change introduces new features or adjustments, highlight them here.

Related #3425

Copilot AI review requested due to automatic review settings August 25, 2025 13:03
@Abhirup-99 Abhirup-99 requested a review from a team as a code owner August 25, 2025 13:03
@welcome
Copy link

welcome bot commented Aug 25, 2025

Thanks for opening this pull request! 🎉 Please check out our contributing guidelines. If you need help or want to chat with us, join us on Discord https://gofiber.io/discord

@coderabbitai
Copy link
Contributor

coderabbitai bot commented Aug 25, 2025

Note

Other AI code review bot(s) detected

CodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review.

Walkthrough

Adds end-to-end response-body streaming: client API (get/set), transport interface and implementations, Response.BodyStream/IsStreaming accessors, Save() streaming branches, request BodyStream propagation in core, tests, and docs.

Changes

Cohort / File(s) Summary
Client Streaming API
\client/client.go``
Adds StreamResponseBody() bool and SetStreamResponseBody(enable bool) *Client that proxy streaming control to the transport; makes SetCookieJar chainable.
Response Streaming Support
\client/response.go``
Adds BodyStream() io.Reader and IsStreaming() bool; updates Save() to use streaming-aware branches (BodyStream() + io.Copy) and unifies save error messages.
Response Streaming Tests
\client/response_test.go``
Adds extensive streaming tests (BodyStream, fallback, IsStreaming, Save to file/writer, chunked/SSE/progressive streams, writer error/close helpers).
Transport Layer Streaming
\client/transport.go``
Extends httpClientTransport with StreamResponseBody() / SetStreamResponseBody(enable bool); implements for standardClientTransport, hostClientTransport, and lbClientTransport with traversal/propagation logic.
Core Request/Response Flow
\client/core.go``
Propagates request BodyStream into fasthttp request (SetBodyStream with ContentLength), nil-guards ReleaseResponse, and safely swaps/releases RawResponse.
Transport Test Stubs
\client/core_test.go``
Adds StreamResponseBody() and SetStreamResponseBody(_ bool) stubs on blockingErrTransport to satisfy new transport API in tests.
Transport Tests
\client/transport_test.go``
Adds unit tests validating streaming flag default/enable/disable and propagation across standard, host, LB, nested, and mixed transports.
Client Tests
\client/client_test.go``
Adds tests for Client streaming getters/setters and propagation to underlying transports.
Context Tests
\ctx_test.go``
Tests now call app.Test(req) with a per-call TestConfig{Timeout: 5 * time.Second}; no logic change beyond test timeout.
Docs
\docs/client/rest.md``
Documents StreamResponseBody() and SetStreamResponseBody(stream bool) in the client API docs.

Sequence Diagram(s)

sequenceDiagram
    participant User
    participant Client
    participant Transport
    participant Core
    participant fasthttp as fasthttp.Client
    participant Response

    User->>Client: SetStreamResponseBody(true)
    Client->>Transport: SetStreamResponseBody(true)
    Transport->>fasthttp: set StreamResponseBody flag

    User->>Client: Do(request)
    Client->>Core: executeRequest(...)
    Core->>fasthttp: Do()/DoDeadline(...)
    fasthttp-->>Core: *fasthttp.Response

    rect rgb(240,250,240)
    Note over Core: if request has BodyStream -> propagate
    Core->>fasthttp: SetBodyStream(contentLen, reader)
    end

    Core-->>Client: build Response (with RawResponse)
    User->>Response: IsStreaming()
    Response-->>User: true/false

    alt streaming
        User->>Response: BodyStream()
        Response-->>User: io.Reader (stream)
        User->>Response: Save(dest)
        Response->>Response: io.Copy(BodyStream -> dest)
    else non-streaming
        User->>Response: Body()
        Response-->>User: []byte
        User->>Response: Save(dest)
        Response->>Response: io.Copy(bytes.Reader -> dest)
    end
Loading

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~25 minutes

  • Focus areas:
    • client/transport.go: traversal and propagation logic in lbClientTransport (nested balancers).
    • client/core.go: RawResponse swap and nil-guarded ReleaseResponse.
    • client/response.go: Save() streaming vs non-streaming branches and resource handling.
    • client/response_test.go & client/transport_test.go: new streaming tests (timing-sensitive).

Possibly related issues

Possibly related PRs

Suggested reviewers

  • sixcolors
  • gaby
  • ReneWerner87

Poem

🐇
I nibble bytes that trickle by,
I pass the stream with hop and cheer,
From client burrow to the sky,
Small paws, big pipelines near. 🥕

Pre-merge checks and finishing touches

❌ Failed checks (1 warning, 1 inconclusive)
Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 18.75% which is insufficient. The required threshold is 80.00%. You can run @coderabbitai generate docstrings to improve docstring coverage.
Description check ❓ Inconclusive The description covers the main changes and problem being addressed, but it duplicates the template header and does not follow the expected checklist format with marked items. Complete the PR description template by adding checked/unchecked checkboxes for benchmarks, documentation, changelog, migration guide, type of change, and pre-submission checklist items as specified in the template.
✅ Passed checks (1 passed)
Check name Status Explanation
Title check ✅ Passed The title clearly summarizes the main feature being added (StreamResponseBody support for the Client), which aligns with the core changes throughout the changeset.
✨ Finishing touches
  • 📝 Generate docstrings
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

Copy link
Contributor

@gemini-code-assist gemini-code-assist bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Summary of Changes

Hello @Abhirup-99, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed!

This pull request introduces a significant enhancement to the client by adding support for streaming HTTP response bodies. This allows consumers to process large responses or server-sent events incrementally, preventing excessive memory consumption. The changes include new configuration options at both the client and individual request levels, along with the necessary logic to facilitate this streaming behavior and comprehensive test coverage.

Highlights

  • Client-level Streaming Configuration: Introduced a streamResponseBody field and corresponding SetStreamResponseBody and StreamResponseBody methods to the Client struct, enabling global control over response body streaming.
  • Request-level Streaming Override: Added a streamResponseBody field and methods to the Request struct, allowing per-request configuration that can override the client's default streaming setting.
  • Dynamic Streaming Execution: Modified the core request execution logic to dynamically set and restore the underlying HTTP client's streaming option based on the active request or client configuration.
  • Streamed Body Access: Implemented a BodyStream() method on the Response struct, providing an io.Reader interface for efficient, stream-based consumption of response bodies.
  • Comprehensive Test Coverage: Added extensive unit tests to validate the new streaming functionality, covering scenarios like server-sent events, large responses, default settings, overrides, and method chaining.
Using Gemini Code Assist

The full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips.

Invoking Gemini

You can request assistance from Gemini at any point in your pull request via creating an issue comment (i.e. comment on the pull request page) using either /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands.

Feature Command Description
Code Review /gemini review Performs a code review for the current pull request in its current state.
Pull Request Summary /gemini summary Provides a summary of the current pull request in its current state.
Comment @gemini-code-assist Responds in comments when explicitly tagged, both in issue comments and review comments.
Help /gemini help Displays a list of available commands.

Customization

To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a .gemini/ folder in the base of the repository. Detailed instructions can be found here.

Limitations & Feedback

Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here.

You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension.

Footnotes

  1. Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution.

@Abhirup-99
Copy link
Author

I will update the documentation if the approach seems sound enough.

Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull Request Overview

This pull request adds support for streaming HTTP response bodies in the client library, enabling responses to be read as streams instead of being fully loaded into memory. This is particularly useful for handling large responses or server-sent events.

  • Added streaming configuration at both client and request levels with override capability
  • Implemented BodyStream() method on Response struct to provide streaming access
  • Added comprehensive test coverage for various streaming scenarios

Reviewed Changes

Copilot reviewed 6 out of 6 changed files in this pull request and generated 2 comments.

Show a summary per file
File Description
client/client.go Adds streamResponseBody field and configuration methods to Client struct
client/request.go Adds per-request streaming configuration with client-level fallback
client/core.go Updates request execution to handle streaming option setting and restoration
client/response.go Implements BodyStream method for streaming response body access
client/client_test.go Comprehensive tests for client-level streaming configuration and behavior
client/response_test.go Tests for response streaming functionality including large data handling

client/core.go Outdated
Comment on lines 84 to 93
// Set StreamResponseBody option from request setting (falls back to client setting)
originalStreamResponseBody := c.client.fasthttp.StreamResponseBody
c.client.fasthttp.StreamResponseBody = c.req.StreamResponseBody()

var err error
go func() {
respv := fasthttp.AcquireResponse()
defer func() {
// Restore original StreamResponseBody setting
c.client.fasthttp.StreamResponseBody = originalStreamResponseBody
Copy link

Copilot AI Aug 25, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The StreamResponseBody setting is being modified on the shared client instance without proper synchronization. In a multi-threaded environment, concurrent requests could interfere with each other's streaming configuration. Consider using per-request client instances or adding proper locking mechanisms.

Suggested change
// Set StreamResponseBody option from request setting (falls back to client setting)
originalStreamResponseBody := c.client.fasthttp.StreamResponseBody
c.client.fasthttp.StreamResponseBody = c.req.StreamResponseBody()
var err error
go func() {
respv := fasthttp.AcquireResponse()
defer func() {
// Restore original StreamResponseBody setting
c.client.fasthttp.StreamResponseBody = originalStreamResponseBody
// Create a shallow copy of the fasthttp.Client to avoid data races on StreamResponseBody
clientCopy := *c.client.fasthttp
clientCopy.StreamResponseBody = c.req.StreamResponseBody()
var err error
go func() {
respv := fasthttp.AcquireResponse()
defer func() {

Copilot uses AI. Check for mistakes.
client/core.go Outdated
Comment on lines 84 to 93
// Set StreamResponseBody option from request setting (falls back to client setting)
originalStreamResponseBody := c.client.fasthttp.StreamResponseBody
c.client.fasthttp.StreamResponseBody = c.req.StreamResponseBody()

var err error
go func() {
respv := fasthttp.AcquireResponse()
defer func() {
// Restore original StreamResponseBody setting
c.client.fasthttp.StreamResponseBody = originalStreamResponseBody
Copy link

Copilot AI Aug 25, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The restoration of the original StreamResponseBody setting in the defer function may not execute in case of panics or early returns, potentially leaving the client in an inconsistent state. Additionally, this restoration happens in a goroutine which could create race conditions with other concurrent requests.

Suggested change
// Set StreamResponseBody option from request setting (falls back to client setting)
originalStreamResponseBody := c.client.fasthttp.StreamResponseBody
c.client.fasthttp.StreamResponseBody = c.req.StreamResponseBody()
var err error
go func() {
respv := fasthttp.AcquireResponse()
defer func() {
// Restore original StreamResponseBody setting
c.client.fasthttp.StreamResponseBody = originalStreamResponseBody
// Use a per-request copy of the fasthttp.Client to avoid data races on StreamResponseBody
clientCopy := *c.client.fasthttp
clientCopy.StreamResponseBody = c.req.StreamResponseBody()
var err error
go func() {
respv := fasthttp.AcquireResponse()
defer func() {

Copilot uses AI. Check for mistakes.
@gemini-code-assist
Copy link
Contributor

Warning

Gemini encountered an error creating the review. You can try again by commenting /gemini review.

@gaby gaby added this to v3 Aug 25, 2025
@gaby gaby added this to the v3 milestone Aug 25, 2025
@gaby gaby changed the title Add stream response body support 🔥 feat: Add StreamResponseBody support for the Client Aug 25, 2025
Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 6

🧹 Nitpick comments (4)
client/request.go (1)

594-605: Effective value resolution is correct and robust

Falls back to the client setting when unset; default false if no client is attached yet. Consider documenting this precedence in the method comment for discoverability.

client/client_test.go (3)

1750-1767: Optional: make SSE handler actually stream (flush between events) for stronger signal

Right now the handler writes three messages and returns. This can still pass even if the server buffers and sends a single chunk. Consider using SetBodyStreamWriter and flushing between writes to exercise true streaming behavior. Example:

app.Get("/sse", func(c fiber.Ctx) error {
	c.Set("Content-Type", "text/event-stream")
	c.Set("Cache-Control", "no-cache")
	c.Set("Connection", "keep-alive")

	c.Context().SetBodyStreamWriter(func(w *bufio.Writer) {
		msgs := []string{"data: message 1\n\n", "data: message 2\n\n", "data: message 3\n\n"}
		for _, m := range msgs {
			_, _ = w.WriteString(m)
			_ = w.Flush()
			time.Sleep(10 * time.Millisecond)
		}
	})
	return nil
})

This reduces false positives where streaming isn’t actually occurring.


1836-1841: Strengthen fallback contract when streaming is disabled by reading BodyStream

You assert BodyStream() is non-nil, but don’t verify it yields the same content as Body() in the fallback path. Read it and compare.

Apply this diff:

 	body := resp.Body()
 	require.Equal(t, "Hello, World!", string(body))

 	bodyStream := resp.BodyStream()
 	require.NotNil(t, bodyStream)
+	streamed, err := io.ReadAll(bodyStream)
+	require.NoError(t, err)
+	require.Equal(t, body, streamed)

1854-1883: Please add a follow-up request and Reset() behavior test to guard against per-request overrides leaking

You’ve already verified the getter/inheritance logic for StreamResponseBody, but we’re still missing two important checks in client/client_test.go around lines 1854–1883:

  • A second request on the same client after an override, to ensure the per-request override doesn’t persist.
  • Exercising Request.Reset() to confirm it clears any per-request streaming flag and falls back to the client default.

Suggested diff to insert after the existing assertions:

 	req2 := client.R().SetStreamResponseBody(false)
 	require.False(t, req2.StreamResponseBody())

+	// Follow-up call: ensure the override didn’t leak into actual behavior.
+	resp2b, err := req2.Get("http://" + addr + "/test")
+	require.NoError(t, err)
+	defer resp2b.Close()
+	require.Equal(t, "Hello, World!", string(resp2b.Body()))

 	clientWithStreaming := New().SetStreamResponseBody(true)
 	req3 := clientWithStreaming.R()
 	require.True(t, req3.StreamResponseBody()) // Should inherit from client
 	req4 := client.R().
 		SetStreamResponseBody(true).
 		SetTimeout(time.Second * 5).
 		SetStreamResponseBody(false)
 	require.False(t, req4.StreamResponseBody())
+
+	// After Reset, per-request override should be cleared and revert to client setting (false).
+	reqReset := client.R().SetStreamResponseBody(true)
+	reqReset.Reset()
+	require.False(t, reqReset.StreamResponseBody())

This will catch regressions where the underlying fasthttp flag isn’t properly restored and confirm Reset() clears the override.

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

💡 Knowledge Base configuration:

  • MCP integration is disabled by default for public repositories
  • Jira integration is disabled by default for public repositories
  • Linear integration is disabled by default for public repositories

You can enable these sources in your CodeRabbit configuration.

📥 Commits

Reviewing files that changed from the base of the PR and between 64a7113 and a725b97.

📒 Files selected for processing (6)
  • client/client.go (2 hunks)
  • client/client_test.go (1 hunks)
  • client/core.go (1 hunks)
  • client/request.go (3 hunks)
  • client/response.go (1 hunks)
  • client/response_test.go (1 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
client/response_test.go (1)
client/client.go (2)
  • Get (706-708)
  • New (647-652)
client/client_test.go (1)
client/client.go (2)
  • New (647-652)
  • Get (706-708)
🔇 Additional comments (6)
client/request.go (3)

70-72: Good choice: tri-state per-request override

Using a pointer to bool for streamResponseBody cleanly distinguishes unset from explicit true/false. LGTM.


606-612: Chainable setter is fine

No issues.


679-679: Reset clears the override as expected

Resetting streamResponseBody to nil ensures subsequent requests inherit the client default. LGTM.

client/client_test.go (3)

1737-1745: LGTM: basic client-level toggle coverage is solid

Covers default false, enable, disable. Clear, fast, and deterministic.


1793-1819: LGTM: large-response streaming vs non-streaming parity

Good end-to-end validation that BodyStream() delivers identical data to Body() for large payloads, with streaming enabled/disabled. Using 1 MiB is a reasonable balance for CI.


1846-1852: LGTM: chainable method precedence

Confirms last-set-wins across chained setters with unrelated setters in between. Clear and sufficient.

Comment on lines 1781 to 2320
buffer := make([]byte, 1024)
n, err := bodyStream.Read(buffer)
require.NoError(t, err)
require.Greater(t, n, 0)

content := string(buffer[:n])
require.Contains(t, content, "data: message 1")
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion

Prevent flaky read assertion: allow io.EOF when n > 0

io.Reader is allowed to return (n > 0, err == io.EOF). Requiring NoError can intermittently fail. Accept EOF if bytes were read.

Apply this diff:

-	buffer := make([]byte, 1024)
-	n, err := bodyStream.Read(buffer)
-	require.NoError(t, err)
-	require.Greater(t, n, 0)
+	buffer := make([]byte, 1024)
+	n, err := bodyStream.Read(buffer)
+	require.Greater(t, n, 0)
+	if err != nil {
+		require.ErrorIs(t, err, io.EOF)
+	}
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
buffer := make([]byte, 1024)
n, err := bodyStream.Read(buffer)
require.NoError(t, err)
require.Greater(t, n, 0)
content := string(buffer[:n])
require.Contains(t, content, "data: message 1")
}
buffer := make([]byte, 1024)
n, err := bodyStream.Read(buffer)
require.Greater(t, n, 0)
if err != nil {
require.ErrorIs(t, err, io.EOF)
}
content := string(buffer[:n])
require.Contains(t, content, "data: message 1")
}
🤖 Prompt for AI Agents
In client/client_test.go around lines 1781-1788, the test currently requires
NoError on bodyStream.Read which is flaky because io.Reader may return (n>0,
err==io.EOF); change the assertion to allow io.EOF when bytes were read: after
reading and asserting n>0, assert either err==nil or errors.Is(err, io.EOF) (or
use require.NoError when err==nil else require.True(errors.Is(err, io.EOF))),
and add imports for "io" and/or "errors" if not already present.

Comment on lines 542 to 1019
func Test_Response_BodyStream(t *testing.T) {
t.Parallel()

server := startTestServer(t, func(app *fiber.App) {
app.Get("/stream", func(c fiber.Ctx) error {
return c.SendString("streaming data")
})
app.Get("/large", func(c fiber.Ctx) error {
data := make([]byte, 1024)
for i := range data {
data[i] = byte('A' + i%26)
}
return c.Send(data)
})
})
defer server.stop()

t.Run("basic streaming", func(t *testing.T) {
client := New().SetDial(server.dial()).SetStreamResponseBody(true)

resp, err := client.Get("http://example.com/stream")
require.NoError(t, err)
defer resp.Close()
bodyStream := resp.BodyStream()
require.NotNil(t, bodyStream)
data, err := io.ReadAll(bodyStream)
require.NoError(t, err)
require.Equal(t, "streaming data", string(data))
})

t.Run("large response streaming", func(t *testing.T) {
client := New().SetDial(server.dial()).SetStreamResponseBody(true)
resp, err := client.Get("http://example.com/large")
require.NoError(t, err)
defer resp.Close()
bodyStream := resp.BodyStream()
require.NotNil(t, bodyStream)
buffer := make([]byte, 256)
var totalRead []byte
for {
n, err := bodyStream.Read(buffer)
if n > 0 {
totalRead = append(totalRead, buffer[:n]...)
}
if err == io.EOF {
break
}
require.NoError(t, err)
}
require.Equal(t, 1024, len(totalRead))
for i := 0; i < 1024; i++ {
expected := byte('A' + i%26)
require.Equal(t, expected, totalRead[i])
}
})

t.Run("compare with regular body", func(t *testing.T) {
client1 := New().SetDial(server.dial())
resp1, err := client1.Get("http://example.com/stream")
require.NoError(t, err)
defer resp1.Close()
normalBody := resp1.Body()
client2 := New().SetDial(server.dial()).SetStreamResponseBody(true)
resp2, err := client2.Get("http://example.com/stream")
require.NoError(t, err)
defer resp2.Close()
streamedBody, err := io.ReadAll(resp2.BodyStream())
require.NoError(t, err)
require.Equal(t, normalBody, streamedBody)
})
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

Fix flakiness: don’t retain Body() buffer after closing the Response

In “compare with regular body”, normalBody references the internal buffer of resp1. After resp1.Close(), that memory can be reused by the pool, leading to flaky comparisons. Copy the bytes before closing.

 	t.Run("compare with regular body", func(t *testing.T) {
 		client1 := New().SetDial(server.dial())
 		resp1, err := client1.Get("http://example.com/stream")
 		require.NoError(t, err)
 		defer resp1.Close()
-		normalBody := resp1.Body()
+		normalBody := append([]byte(nil), resp1.Body()...)
 		client2 := New().SetDial(server.dial()).SetStreamResponseBody(true)
 		resp2, err := client2.Get("http://example.com/stream")
 		require.NoError(t, err)
 		defer resp2.Close()
 		streamedBody, err := io.ReadAll(resp2.BodyStream())
 		require.NoError(t, err)
 		require.Equal(t, normalBody, streamedBody)
 	})

Optional coverage: add an assertion that len(resp.Body()) == 0 when streaming is enabled to codify the documented behavior.

📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
func Test_Response_BodyStream(t *testing.T) {
t.Parallel()
server := startTestServer(t, func(app *fiber.App) {
app.Get("/stream", func(c fiber.Ctx) error {
return c.SendString("streaming data")
})
app.Get("/large", func(c fiber.Ctx) error {
data := make([]byte, 1024)
for i := range data {
data[i] = byte('A' + i%26)
}
return c.Send(data)
})
})
defer server.stop()
t.Run("basic streaming", func(t *testing.T) {
client := New().SetDial(server.dial()).SetStreamResponseBody(true)
resp, err := client.Get("http://example.com/stream")
require.NoError(t, err)
defer resp.Close()
bodyStream := resp.BodyStream()
require.NotNil(t, bodyStream)
data, err := io.ReadAll(bodyStream)
require.NoError(t, err)
require.Equal(t, "streaming data", string(data))
})
t.Run("large response streaming", func(t *testing.T) {
client := New().SetDial(server.dial()).SetStreamResponseBody(true)
resp, err := client.Get("http://example.com/large")
require.NoError(t, err)
defer resp.Close()
bodyStream := resp.BodyStream()
require.NotNil(t, bodyStream)
buffer := make([]byte, 256)
var totalRead []byte
for {
n, err := bodyStream.Read(buffer)
if n > 0 {
totalRead = append(totalRead, buffer[:n]...)
}
if err == io.EOF {
break
}
require.NoError(t, err)
}
require.Equal(t, 1024, len(totalRead))
for i := 0; i < 1024; i++ {
expected := byte('A' + i%26)
require.Equal(t, expected, totalRead[i])
}
})
t.Run("compare with regular body", func(t *testing.T) {
client1 := New().SetDial(server.dial())
resp1, err := client1.Get("http://example.com/stream")
require.NoError(t, err)
defer resp1.Close()
normalBody := resp1.Body()
client2 := New().SetDial(server.dial()).SetStreamResponseBody(true)
resp2, err := client2.Get("http://example.com/stream")
require.NoError(t, err)
defer resp2.Close()
streamedBody, err := io.ReadAll(resp2.BodyStream())
require.NoError(t, err)
require.Equal(t, normalBody, streamedBody)
})
}
t.Run("compare with regular body", func(t *testing.T) {
client1 := New().SetDial(server.dial())
resp1, err := client1.Get("http://example.com/stream")
require.NoError(t, err)
defer resp1.Close()
normalBody := append([]byte(nil), resp1.Body()...)
client2 := New().SetDial(server.dial()).SetStreamResponseBody(true)
resp2, err := client2.Get("http://example.com/stream")
require.NoError(t, err)
defer resp2.Close()
streamedBody, err := io.ReadAll(resp2.BodyStream())
require.NoError(t, err)
require.Equal(t, normalBody, streamedBody)
})
🤖 Prompt for AI Agents
In client/response_test.go around lines 542 to 612, the "compare with regular
body" subtest keeps normalBody as a reference to resp1's internal buffer and
then closes resp1, which can make the comparison flaky because the buffer may be
reused; fix by making a copy of resp1.Body() (e.g., allocate and copy the bytes)
before calling resp1.Close(), then use that copied slice for the equality check;
optionally add an assertion that when streaming is enabled the non-streaming
response's Body() is emptied (len(resp.Body()) == 0) to codify documented
behavior.

@gaby
Copy link
Member

gaby commented Aug 25, 2025

@Abhirup-99 The tests are failing. You can run them locally by running make tests

@Abhirup-99
Copy link
Author

@gaby fixed the failing tests

@codecov
Copy link

codecov bot commented Aug 25, 2025

Codecov Report

❌ Patch coverage is 96.42857% with 2 lines in your changes missing coverage. Please review.
✅ Project coverage is 92.20%. Comparing base (bc74824) to head (402be56).
⚠️ Report is 17 commits behind head on main.

Files with missing lines Patch % Lines
client/core.go 80.00% 1 Missing and 1 partial ⚠️
Additional details and impacted files
@@           Coverage Diff           @@
##             main    #3711   +/-   ##
=======================================
  Coverage   92.20%   92.20%           
=======================================
  Files         115      115           
  Lines        9744     9802   +58     
=======================================
+ Hits         8984     9038   +54     
- Misses        485      487    +2     
- Partials      275      277    +2     
Flag Coverage Δ
unittests 92.20% <96.42%> (+<0.01%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 0

♻️ Duplicate comments (3)
client/client.go (2)

538-557: Reset should also clear streamResponseBody to default false

Client.Reset clears most flags but leaves streamResponseBody unchanged, leading to surprising behavior post-reset.

 func (c *Client) Reset() {
 	c.fasthttp = &fasthttp.Client{}
 	c.baseURL = ""
 	c.timeout = 0
 	c.userAgent = ""
 	c.referer = ""
 	c.retryConfig = nil
 	c.debug = false
+	c.streamResponseBody = false

58-61: Guard temporary per-request flips of fasthttp.Client.StreamResponseBody with a dedicated mutex on Client

If core executes by toggling the shared fasthttp.Client.StreamResponseBody between requests, concurrent requests can race. Add a small private mutex dedicated to this toggle. This keeps the public API unchanged and avoids overloading c.mu, which guards other state.

Apply:

 type Client struct {
  	logger   log.CommonLogger
  	fasthttp *fasthttp.Client
@@
-	timeout            time.Duration
-	mu                 sync.RWMutex
-	debug              bool
-	streamResponseBody bool
+	timeout            time.Duration
+	mu                 sync.RWMutex
+	debug              bool
+	streamResponseBody bool
+	// protects temporary flips of fasthttp.Client.StreamResponseBody during per-request execution
+	streamRespMu       sync.Mutex
 }

To confirm whether the toggle approach is used (vs. cloning a temp fasthttp.Client), run:

#!/bin/bash
# Inspect how StreamResponseBody is applied during execution.
rg -n -C2 --type=go 'StreamResponseBody' client/core.go client | sed -n '1,200p'
client/response_test.go (1)

598-611: Fix flakiness: copy Body() before closing resp1

normalBody aliases resp1’s internal buffer; after Close(), pooled buffers may be reused, causing nondeterministic failures. Copy the bytes before closing.

-		defer resp1.Close()
-		normalBody := resp1.Body()
+		defer resp1.Close()
+		normalBody := append([]byte(nil), resp1.Body()...)
🧹 Nitpick comments (2)
client/client.go (2)

439-451: API shape LGTM; document precedence explicitly in the comment

The getter/setter are clear and chainable. Add one line noting that a request-level setting overrides the client-level default for discoverability.

 // SetStreamResponseBody enables or disables response body streaming.
 // When enabled, the response body can be read as a stream using BodyStream()
 // instead of being fully loaded into memory. This is useful for large responses
-// or server-sent events.
+// or server-sent events. Note: a per-request setting (Request.SetStreamResponseBody)
+// overrides this client-level default.
 func (c *Client) SetStreamResponseBody(enable bool) *Client {
 	c.streamResponseBody = enable
 	return c
 }

563-575: Optional: plumb StreamResponseBody through Config for convenience

Many helpers accept Config; adding an optional pointer makes it easy to set streaming per call without constructing a Request manually.

 type Config struct {
 	Ctx          context.Context //nolint:containedctx // It's needed to be stored in the config.
 	Body         any
 	Header       map[string]string
 	Param        map[string]string
 	Cookie       map[string]string
 	PathParam    map[string]string
 	FormData     map[string]string
 	UserAgent    string
 	Referer      string
 	File         []*File
 	Timeout      time.Duration
 	MaxRedirects int
+	// Optional per-request override; when set, overrides the client default.
+	StreamResponseBody *bool
 }
@@
 	if cfg.MaxRedirects != 0 {
 		req.SetMaxRedirects(cfg.MaxRedirects)
 	}
 
+	if cfg.StreamResponseBody != nil {
+		req.SetStreamResponseBody(*cfg.StreamResponseBody)
+	}
+
 	if cfg.Body != nil {
 		req.SetJSON(cfg.Body)
 		return
 	}

Also applies to: 577-635

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

💡 Knowledge Base configuration:

  • MCP integration is disabled by default for public repositories
  • Jira integration is disabled by default for public repositories
  • Linear integration is disabled by default for public repositories

You can enable these sources in your CodeRabbit configuration.

📥 Commits

Reviewing files that changed from the base of the PR and between a725b97 and 9c12637.

📒 Files selected for processing (6)
  • client/client.go (2 hunks)
  • client/client_test.go (1 hunks)
  • client/core.go (2 hunks)
  • client/request.go (3 hunks)
  • client/response.go (1 hunks)
  • client/response_test.go (1 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
  • client/response.go
  • client/client_test.go
  • client/core.go
🔇 Additional comments (5)
client/response_test.go (1)

542-596: Solid coverage for basic and large streaming paths

The test server and the two subtests validate BodyStream() on small and larger responses. Clear and deterministic.

client/request.go (4)

70-72: *Good choice using bool to represent tri-state (unset vs true/false)

This enables request-level override while preserving client default. No action needed.


594-605: Precedence and fallback logic reads correctly

Returns request override when set; otherwise falls back to client-level; defaults to false if no client. Matches the intended design.


606-612: Setter is fine; pointer escape is safe

Using &enable is idiomatic (escapes to heap). Chainable API is consistent with others.


679-679: Reset correctly clears the per-request override

Resetting streamResponseBody to nil ensures the request returns to default behavior. Good.

@gaby gaby moved this to In Progress in v3 Aug 25, 2025
Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 2

🧹 Nitpick comments (1)
client/core.go (1)

132-141: Minor duplication: unify Do vs DoRedirects callsites

The four callsites using fastHTTPClient.Do/DoRedirects under retry and non-retry paths duplicate logic. A tiny helper makes it harder to regress:

- if cfg != nil {
-   err = retry.NewExponentialBackoff(*cfg).Retry(func() error {
-     if shouldRedirect {
-       return fastHTTPClient.DoRedirects(reqv, respv, c.req.maxRedirects)
-     }
-     return fastHTTPClient.Do(reqv, respv)
-   })
- } else {
-   if shouldRedirect {
-     err = fastHTTPClient.DoRedirects(reqv, respv, c.req.maxRedirects)
-   } else {
-     err = fastHTTPClient.Do(reqv, respv)
-   }
- }
+call := func() error {
+  if shouldRedirect {
+    return fastHTTPClient.DoRedirects(reqv, respv, c.req.maxRedirects)
+  }
+  return fastHTTPClient.Do(reqv, respv)
+}
+if cfg != nil {
+  err = retry.NewExponentialBackoff(*cfg).Retry(call)
+} else {
+  err = call()
+}

Low impact, but improves readability.

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

💡 Knowledge Base configuration:

  • MCP integration is disabled by default for public repositories
  • Jira integration is disabled by default for public repositories
  • Linear integration is disabled by default for public repositories

You can enable these sources in your CodeRabbit configuration.

📥 Commits

Reviewing files that changed from the base of the PR and between 9c12637 and 4d32c17.

📒 Files selected for processing (6)
  • client/client.go (2 hunks)
  • client/client_test.go (1 hunks)
  • client/core.go (2 hunks)
  • client/request.go (3 hunks)
  • client/response.go (1 hunks)
  • client/response_test.go (1 hunks)
🚧 Files skipped from review as they are similar to previous changes (5)
  • client/response.go
  • client/client.go
  • client/request.go
  • client/response_test.go
  • client/client_test.go
🧰 Additional context used
🧬 Code graph analysis (1)
client/core.go (1)
client/client.go (1)
  • Client (32-62)
🔇 Additional comments (3)
client/core.go (3)

84-118: Kudos: avoids flipping shared fasthttp.Client.StreamResponseBody

Not mutating the shared client’s flag sidesteps the prior data race concerns. Good move. If you adopt the dual-client approach suggested above, you’ll keep this benefit while restoring pooling.


149-151: No action needed: CopyTo + ReleaseResponse ordering is safe

Verified that fasthttp.Response.CopyTo performs a synchronous copy of the body into resp.RawResponse (rather than merely reassigning a stream pointer), and once CopyTo returns, you can immediately call ReleaseResponse(respv) without affecting the destination’s BodyStream. The existing code at client/core.go (lines 149–151) is correct and requires no changes.


88-118: Verify comprehensive cloning of fasthttp.Client fields
I wasn’t able to locate client/core.go in the repository, but manually copying each exported field from fasthttp.Client is brittle and prone to omissions (e.g., new fields like MaxConnWaitTimeout, RetryIf, etc., won’t be copied and can silently change behavior). Please verify that you:

  • Mirror all exported fields when creating a temporary client for per-request overrides.
  • Centralize this logic in a helper (e.g., reflection-based) to automatically pick up new fields.
  • Add a unit test that fails if any exported field in fasthttp.Client isn’t present in the clone.
  • Or consider toggling StreamResponseBody on the shared client under a lock instead of cloning.

Let me know if you’d like a PR-ready helper and reflection-based test snippet for this.

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 0

♻️ Duplicate comments (3)
client/response.go (2)

92-101: Tighten BodyStream docs: lifetime, connection reuse, and Body() emptiness when streaming

The code is fine. Please clarify the contract and call order to prevent misuse with pooled responses and fasthttp’s connection reuse.

Apply this doc tweak:

-// BodyStream returns the response body as a stream reader.
-// Note: When using BodyStream(), the response body is not copied to memory,
-// so calling Body() afterwards may return an empty slice.
+// BodyStream returns an io.Reader for the response body.
+// If streaming was enabled (via Request.SetStreamResponseBody, Client.SetStreamResponseBody,
+// or the underlying fasthttp.Client.StreamResponseBody), the reader yields bytes directly from
+// the connection without buffering in memory.
+//
+// Important:
+// - Consume the reader before calling Response.Close().
+// - Read the stream to EOF to allow connection reuse by the underlying client.
+// - When streaming is enabled, Body() (and helpers like String/JSON/XML/CBOR/Save) may be empty;
+//   prefer io.Copy with BodyStream() for large responses.
+// If streaming is not enabled, BodyStream returns a bytes.Reader over Body().
 func (r *Response) BodyStream() io.Reader {

150-165: Save() ignores streaming and closes caller’s writer — stream from BodyStream() and don’t close external writers

  • Using Body() makes Save() write an empty file/output when streaming is enabled.
  • Closing an io.Writer that the function didn’t open is surprising and can break callers.

Apply this minimal change:

-    if _, err = io.Copy(outFile, bytes.NewReader(r.Body())); err != nil {
+    if _, err = io.Copy(outFile, r.BodyStream()); err != nil {
       return fmt.Errorf("failed to write response body to file: %w", err)
     }
@@
-  case io.Writer:
-    if _, err := io.Copy(p, bytes.NewReader(r.Body())); err != nil {
+  case io.Writer:
+    if _, err := io.Copy(p, r.BodyStream()); err != nil {
       return fmt.Errorf("failed to write response body to io.Writer: %w", err)
     }
-    defer func() {
-      if pc, ok := p.(io.WriteCloser); ok {
-        _ = pc.Close() //nolint:errcheck // not needed
-      }
-    }()
     return nil

Follow-up (optional): consider adding a short note in Save()’s doc that it respects streaming and doesn’t close external writers.

client/response_test.go (1)

606-629: Avoid retaining Body()’s internal buffer across Close; also assert Body() is empty when streaming

To prevent future flakiness with pooled buffers and to codify the streaming contract, copy the non-streamed body and add an assertion that Body() is empty when streaming is enabled.

Apply:

-    defer resp1.Close()
-    normalBody := resp1.Body()
+    defer resp1.Close()
+    normalBody := append([]byte(nil), resp1.Body()...) // copy before pool reuse
@@
-    streamedBody, err := io.ReadAll(resp2.BodyStream())
+    streamedBody, err := io.ReadAll(resp2.BodyStream())
     require.NoError(t, err)
     require.Equal(t, normalBody, streamedBody)
+    // streaming mode shouldn't buffer Body()
+    require.Len(t, resp2.Body(), 0)
🧹 Nitpick comments (1)
client/response_test.go (1)

542-629: Add a streaming Save() regression test (uses BodyStream and does not close external writer)

Given the Save() fix, add coverage to ensure we don’t regress: the writer must not be closed by Save(), and content must be streamed correctly when streaming is enabled.

Insert this new subtest at the end of Test_Response_BodyStream (before the final closing brace):

+   t.Run("Save streams and does not close external writer", func(t *testing.T) {
+     t.Parallel()
+
+     server := startTestServer(t, func(app *fiber.App) {
+       app.Get("/stream", func(c fiber.Ctx) error {
+         return c.SendString("streaming data")
+       })
+     })
+     defer server.stop()
+
+     client := New().SetDial(server.dial()).SetStreamResponseBody(true)
+     resp, err := client.Get("http://example.com/stream")
+     require.NoError(t, err)
+     defer resp.Close()
+
+     type trackingBuf struct {
+       bytes.Buffer
+       closed bool
+     }
+     // implements io.WriteCloser to verify Save() does not close it
+     var w = &trackingBuf{}
+     // wrap with NopCloser-like shim
+     writer := struct {
+       io.Writer
+       io.Closer
+     }{
+       Writer: w,
+       Closer: io.CloserFunc(func() error {
+         w.closed = true
+         return nil
+       }),
+     }
+
+     // Save to writer; it must not close it
+     err = resp.Save(writer)
+     require.NoError(t, err)
+     require.Equal(t, "streaming data", w.String())
+     require.False(t, w.closed, "Save must not close external writers")
+   })

If you prefer, we can place a similar subtest under Test_Response_Save.

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

💡 Knowledge Base configuration:

  • MCP integration is disabled by default for public repositories
  • Jira integration is disabled by default for public repositories
  • Linear integration is disabled by default for public repositories

You can enable these sources in your CodeRabbit configuration.

📥 Commits

Reviewing files that changed from the base of the PR and between 4d32c17 and 28342fa.

📒 Files selected for processing (6)
  • client/client.go (2 hunks)
  • client/client_test.go (1 hunks)
  • client/core.go (2 hunks)
  • client/request.go (3 hunks)
  • client/response.go (1 hunks)
  • client/response_test.go (1 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
  • client/client.go
  • client/request.go
  • client/core.go
  • client/client_test.go
🧰 Additional context used
🧬 Code graph analysis (1)
client/response_test.go (1)
client/client.go (2)
  • Get (706-708)
  • New (647-652)

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

♻️ Duplicate comments (3)
client/response.go (2)

92-101: Tighten BodyStream contract; clarify lifetime and fallback behavior

Doc is still a bit vague about ordering/lifetime and the fallback to an in-memory reader. Propose expanding the comment to state the contract explicitly and reduce surprises.

-// BodyStream returns the response body as a stream reader.
-// Note: When using BodyStream(), the response body is not copied to memory,
-// so calling Body() afterwards may return an empty slice.
+// BodyStream returns an io.Reader for the response body.
+//
+// If streaming was enabled (via Request.SetStreamResponseBody, Client.SetStreamResponseBody,
+// or the underlying fasthttp.Client.StreamResponseBody), the reader yields data directly from
+// the connection without buffering. Otherwise, it returns a bytes.Reader over Body().
+//
+// Important:
+// - Consume the reader before calling Response.Close().
+// - When streaming is enabled, Body() (and helpers like String/JSON/XML/CBOR/Save) may observe
+//   an empty body. Prefer io.Copy with BodyStream() for large responses.
 func (r *Response) BodyStream() io.Reader {
   if stream := r.RawResponse.BodyStream(); stream != nil {
     return stream
   }
   // If streaming is not enabled, return a bytes.Reader from the regular body
   return bytes.NewReader(r.RawResponse.Body())
 }

161-172: Make Save() always stream via BodyStream(); also stream when saving to a file

The writer branch partially streams, but the file path branch still buffers via Body(). Since BodyStream() already falls back to a bytes.Reader when streaming isn’t enabled, you can unconditionally copy from BodyStream() in both branches. This avoids large in-memory buffers and simplifies the code.

@@
-    if _, err = io.Copy(outFile, bytes.NewReader(r.Body())); err != nil {
+    if _, err = io.Copy(outFile, r.BodyStream()); err != nil {
       return fmt.Errorf("failed to write response body to file: %w", err)
     }
@@
-  case io.Writer:
-    var err error
-    if r.IsStreaming() {
-      _, err = io.Copy(p, r.BodyStream())
-    } else {
-      _, err = io.Copy(p, bytes.NewReader(r.Body()))
-    }
-
-    if err != nil {
-      return fmt.Errorf("failed to write response body to writer: %w", err)
-    }
-
-    return nil
+  case io.Writer:
+    if _, err := io.Copy(p, r.BodyStream()); err != nil {
+      return fmt.Errorf("failed to write response body to writer: %w", err)
+    }
+    return nil

Follow-up: consider adding/adjusting tests to cover Save(filePath) with streaming enabled (e.g., large response) to ensure we don’t regress. I can draft that if helpful.

client/core.go (1)

84-120: Per-request allocating a new fasthttp.Client loses connection pooling and adds substantial latency/TLS overhead

Constructing a fresh *fasthttp.Client whenever the per-request streaming flag differs discards the shared connection pool and TLS session cache, causing extra TCP/TLS handshakes and reduced throughput under mixed workloads. This will regress performance for SSE/large downloads that toggle streaming on/off across requests.

Consider maintaining two long-lived internal clients (streaming vs non-streaming) on Client and routing each request accordingly. Here's a suggested implementation:

Add to client/client.go:

+// In Client struct:
+streamingFHC *fasthttp.Client // lazily initialized clone with StreamResponseBody=true
+
+// pickFastHTTPClient returns a pooled client configured for the requested streaming mode.
+func (c *Client) pickFastHTTPClient(stream bool) *fasthttp.Client {
+	c.mu.RLock()
+	defer c.mu.RUnlock()
+
+	if stream == c.streamResponseBody {
+		return c.fasthttp
+	}
+	// Lazily build the streaming variant once and reuse it.
+	if stream && c.streamingFHC != nil {
+		return c.streamingFHC
+	}
+	return c.initStreamingClientLocked(stream)
+}
+
+// initStreamingClientLocked clones exported config into a new fasthttp.Client.
+// c.mu must be held by the caller.
+func (c *Client) initStreamingClientLocked(stream bool) *fasthttp.Client {
+	o := c.fasthttp
+	clone := &fasthttp.Client{
+		Dial:                          o.Dial,
+		DialDualStack:                 o.DialDualStack,
+		TLSConfig:                     o.TLSConfig,
+		MaxConnsPerHost:               o.MaxConnsPerHost,
+		MaxIdleConnDuration:           o.MaxIdleConnDuration,
+		MaxConnDuration:               o.MaxConnDuration,
+		ReadTimeout:                   o.ReadTimeout,
+		WriteTimeout:                  o.WriteTimeout,
+		ReadBufferSize:                o.ReadBufferSize,
+		WriteBufferSize:               o.WriteBufferSize,
+		MaxResponseBodySize:           o.MaxResponseBodySize,
+		NoDefaultUserAgentHeader:      o.NoDefaultUserAgentHeader,
+		DisableHeaderNamesNormalizing: o.DisableHeaderNamesNormalizing,
+		DisablePathNormalizing:        o.DisablePathNormalizing,
+		MaxIdemponentCallAttempts:     o.MaxIdemponentCallAttempts,
+		Name:                          o.Name,
+		ConfigureClient:               o.ConfigureClient,
+		StreamResponseBody:            stream,
+	}
+	if stream {
+		c.streamingFHC = clone
+	}
+	return clone
+}

Then simplify this code block:

-	// Determine which client to use - create a new one if StreamResponseBody differs
-	var fastHTTPClient *fasthttp.Client
-	requestStreamResponseBody := c.req.StreamResponseBody()
-	c.client.mu.RLock()
-	clientStream := c.client.streamResponseBody
-	original := c.client.fasthttp
-
-	if requestStreamResponseBody != clientStream {
-		// Request setting differs from client setting, create a temporary client
-
-		fastHTTPClient = &fasthttp.Client{
-			Dial:                          original.Dial,
-			DialDualStack:                 original.DialDualStack,
-			TLSConfig:                     original.TLSConfig,
-			MaxConnsPerHost:               original.MaxConnsPerHost,
-			MaxIdleConnDuration:           original.MaxIdleConnDuration,
-			MaxConnDuration:               original.MaxConnDuration,
-			ReadTimeout:                   original.ReadTimeout,
-			WriteTimeout:                  original.WriteTimeout,
-			ReadBufferSize:                original.ReadBufferSize,
-			WriteBufferSize:               original.WriteBufferSize,
-			MaxResponseBodySize:           original.MaxResponseBodySize,
-			NoDefaultUserAgentHeader:      original.NoDefaultUserAgentHeader,
-			DisableHeaderNamesNormalizing: original.DisableHeaderNamesNormalizing,
-			DisablePathNormalizing:        original.DisablePathNormalizing,
-			MaxIdemponentCallAttempts:     original.MaxIdemponentCallAttempts,
-			Name:                          original.Name,
-			ConfigureClient:               original.ConfigureClient,
-
-			// Request-specific override
-			StreamResponseBody: requestStreamResponseBody,
-		}
-	} else {
-		fastHTTPClient = original
-	}
-	c.client.mu.RUnlock()
+	// Determine which client to use without sacrificing connection pooling
+	requestStreamResponseBody := c.req.StreamResponseBody()
+	fastHTTPClient := c.client.pickFastHTTPClient(requestStreamResponseBody)

This keeps pooling intact and avoids repeated allocation while maintaining thread safety.

🧹 Nitpick comments (1)
client/response.go (1)

103-106: IsStreaming semantics are OK; consider documenting that it reflects fasthttp.BodyStream presence

Minor: IsStreaming simply mirrors RawResponse.BodyStream() != nil. A short note in the comment that this reports whether the underlying fasthttp response is streaming (not whether data remains unread) would avoid confusion.

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

💡 Knowledge Base configuration:

  • MCP integration is disabled by default for public repositories
  • Jira integration is disabled by default for public repositories
  • Linear integration is disabled by default for public repositories

You can enable these sources in your CodeRabbit configuration.

📥 Commits

Reviewing files that changed from the base of the PR and between 28342fa and ddbb831.

📒 Files selected for processing (6)
  • client/client.go (2 hunks)
  • client/client_test.go (1 hunks)
  • client/core.go (2 hunks)
  • client/request.go (3 hunks)
  • client/response.go (2 hunks)
  • client/response_test.go (1 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
  • client/response_test.go
  • client/client_test.go
  • client/client.go
  • client/request.go
🧰 Additional context used
🧬 Code graph analysis (1)
client/core.go (1)
client/client.go (1)
  • Client (32-62)
🔇 Additional comments (1)
client/core.go (1)

133-142: LGTM!

The routing to use fastHTTPClient instead of the hardcoded client is correct and properly handles both retry and non-retry paths with redirects.

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 0

♻️ Duplicate comments (2)
client/response_test.go (2)

704-726: Fix buffer retention issue before closing Response.

Line 718 stores a reference to resp1.Body() in normalBody, then Line 717 defers resp1.Close(). After closing, the internal buffer can be reused by the pool, leading to flaky comparisons.

Apply this diff to copy the bytes before closing:

 	client1 := New().SetDial(server.dial())
 	resp1, err := client1.Get("http://example.com/stream")
 	require.NoError(t, err)
 	defer resp1.Close()
-	normalBody := resp1.Body()
+	normalBody := append([]byte(nil), resp1.Body()...)

779-828: Test name says "incremental reads" but doesn't test incremental behavior.

The subtest uses io.ReadAll (line 818) which reads the entire response at once, not incrementally. Additionally, the server handler doesn't use SetBodyStreamWriter to truly stream the response—it uses ImmediateHeaderFlush which may not force streaming semantics.

To properly test incremental reads:

  1. Use a buffered reader and read line-by-line or in chunks
  2. Use SetBodyStreamWriter on the server to ensure true streaming:
c.Context().SetBodyStreamWriter(func(w *bufio.Writer) {
    messages := []string{
        "data: event 1\n\n",
        "data: event 2\n\n",
        "data: event 3\n\n",
        "data: event 4\n\n",
    }
    for i, msg := range messages {
        _, _ = w.WriteString(msg)
        _ = w.Flush()
        if i < len(messages)-1 {
            time.Sleep(5 * time.Millisecond)
        }
    }
})

Then read incrementally:

r := bufio.NewReader(bodyStream)
for i := 1; i <= 4; i++ {
    line, err := r.ReadString('\n')
    require.NoError(t, err)
    require.Contains(t, line, fmt.Sprintf("data: event %d", i))
    _, _ = r.ReadString('\n') // consume blank line
}
🧹 Nitpick comments (2)
client/response_test.go (2)

728-777: Consider using SetBodyStreamWriter for guaranteed streaming behavior.

The test uses ImmediateHeaderFlush (line 740) which may not guarantee true streaming. For more robust streaming tests, consider using SetBodyStreamWriter:

c.Context().SetBodyStreamWriter(func(w *bufio.Writer) {
    chunks := []string{"chunk1", "chunk2", "chunk3"}
    for i, chunk := range chunks {
        _, _ = w.WriteString(chunk)
        _ = w.Flush()
        if i < len(chunks)-1 {
            time.Sleep(10 * time.Millisecond)
        }
    }
})

This ensures the server actually streams rather than buffering and sending at once.


830-876: Consider using SetBodyStreamWriter for true progressive streaming.

Similar to the chunked test, this test uses ImmediateHeaderFlush which may not force true streaming behavior. Using SetBodyStreamWriter would guarantee progressive writes and better validate the streaming feature.

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 72ece4b and 250b1c2.

📒 Files selected for processing (6)
  • client/client.go (1 hunks)
  • client/client_test.go (1 hunks)
  • client/request.go (1 hunks)
  • client/response.go (2 hunks)
  • client/response_test.go (2 hunks)
  • docs/client/rest.md (1 hunks)
✅ Files skipped from review due to trivial changes (1)
  • client/client_test.go
🚧 Files skipped from review as they are similar to previous changes (3)
  • docs/client/rest.md
  • client/client.go
  • client/response.go
🧰 Additional context used
📓 Path-based instructions (2)
**/*.go

📄 CodeRabbit inference engine (AGENTS.md)

**/*.go: Format Go code using gofumpt (Make target: make format).
Run golangci-lint for linting (Make target: make lint).
Run go vet as part of audit to catch suspicious constructs (Make target: make audit).
Optimize struct field alignment with betteralign (Make target: make betteralign).
Apply gopls modernize to update code patterns (Make target: make modernize).

Files:

  • client/response_test.go
  • client/request.go
**/*_test.go

📄 CodeRabbit inference engine (AGENTS.md)

**/*_test.go: Run the test suite with gotestsum (Make target: make test).
Run benchmarks with go test (Make target: make benchmark).

Files:

  • client/response_test.go
🧬 Code graph analysis (2)
client/response_test.go (2)
client/client.go (1)
  • New (745-750)
client/response.go (1)
  • Response (19-25)
client/request.go (1)
client/client.go (1)
  • Client (37-67)
🔇 Additional comments (11)
client/response_test.go (8)

6-15: LGTM: Standard library imports support new test scenarios.

The new imports are appropriate for the expanded streaming tests and error handling validation.


546-604: LGTM: Good coverage of streaming Save() scenarios.

The tests correctly verify that Save() doesn't close the writer and properly handles copy errors. The error handling and test helpers are well-implemented.


606-638: LGTM: Well-designed test helpers.

Both helper types are focused and appropriate for their use cases. The implementations correctly simulate the behaviors needed for testing.


643-663: LGTM: Basic streaming test is well-structured.

The test correctly enables streaming, verifies the stream is not nil, and validates the content.


665-702: LGTM: Progressive reading properly validates streaming behavior.

This test correctly reads the response in chunks using a buffer and validates both the total size and the data pattern. The loop with explicit buffer reads confirms streaming semantics.


878-917: LGTM: Comprehensive coverage of streaming edge cases.

These subtests effectively cover:

  • Connection interruption and cleanup (lines 878-917)
  • Large response streaming with multiple reads (lines 919-981)
  • Stream object identity guarantees (lines 983-1023)

The tests properly validate streaming semantics through progressive reads and appropriate assertions.

Also applies to: 919-981, 983-1023


1026-1109: LGTM: Fallback behavior is thoroughly tested.

The subtests correctly verify that BodyStream() returns a bytes.Reader when streaming is disabled, and that this fallback works correctly for empty, regular, and large responses. The progressive reading in the "large non-streaming response" test confirms the Reader interface works as expected.


1111-1184: LGTM: Complete coverage of IsStreaming() API.

The tests thoroughly validate the IsStreaming() method and confirm that BodyStream() provides a consistent interface regardless of whether streaming is enabled or disabled. The comparison between streaming and non-streaming clients (lines 1163-1182) is particularly valuable.

client/request.go (3)

605-615: LGTM: DisablePathNormalizing methods follow established patterns.

The getter and setter are well-implemented. The setter correctly propagates the setting to the underlying RawRequest.URI().DisablePathNormalizing (line 613), ensuring consistency across the request chain.


670-697: LGTM: Reset method correctly clears the new field.

Line 683 properly resets disablePathNormalizing to false, maintaining the Reset contract of returning the Request to its default state.


46-68: Confirm make betteralign was run per project guidelines.

The Makefile includes a betteralign target using v0.7.2. The Request struct has been extensively reordered and the new disablePathNormalizing field correctly mirrors the Client struct layout.

However, I cannot verify in this environment whether the field alignment is actually optimal. Please confirm you ran make betteralign (or included it in your build process) to ensure the struct meets the project's optimization standards, as required by the coding guidelines.

@gaby
Copy link
Member

gaby commented Oct 30, 2025

@codex review

Copy link

@chatgpt-codex-connector chatgpt-codex-connector bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

client/client.go Outdated
Comment on lines 529 to 539
// StreamResponseBody returns the current StreamResponseBody setting.
func (c *Client) StreamResponseBody() bool {
return c.fasthttp.StreamResponseBody
}

// SetStreamResponseBody enables or disables response body streaming.
// When enabled, the response body can be read as a stream using BodyStream()
// instead of being fully loaded into memory. This is useful for large responses
// or server-sent events.
func (c *Client) SetStreamResponseBody(enable bool) *Client {
c.fasthttp.StreamResponseBody = enable

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P0 Badge Reference undefined fasthttp client in streaming helpers

StreamResponseBody() and SetStreamResponseBody() try to read and write c.fasthttp.StreamResponseBody, but Client has no fasthttp field—only a transport interface. This code does not compile (*Client has no field or method fasthttp) and therefore the new streaming API cannot be built. The implementation should operate on the existing transport (e.g., via FasthttpClient()/HostClient()/LBClient()) or store the required field on the struct.

Useful? React with 👍 / 👎.

Copy link

@chatgpt-codex-connector chatgpt-codex-connector bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

client/client.go Outdated
Comment on lines 529 to 539
// StreamResponseBody returns the current StreamResponseBody setting.
func (c *Client) StreamResponseBody() bool {
return c.fasthttp.StreamResponseBody
}

// SetStreamResponseBody enables or disables response body streaming.
// When enabled, the response body can be read as a stream using BodyStream()
// instead of being fully loaded into memory. This is useful for large responses
// or server-sent events.
func (c *Client) SetStreamResponseBody(enable bool) *Client {
c.fasthttp.StreamResponseBody = enable

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P0 Badge Fix compile error when toggling streaming flag

Both StreamResponseBody() and SetStreamResponseBody() attempt to read and write c.fasthttp.StreamResponseBody, but Client has no fasthttp field—only the transport interface. As written, the file does not build (*Client has no field or method fasthttp). To expose this option you must obtain the underlying fasthttp client via the transport (e.g. FasthttpClient(), HostClient(), LBClient()) and set their StreamResponseBody flags instead of referencing a nonexistent field.

Useful? React with 👍 / 👎.

@grivera64
Copy link
Member

PR #3830 got merged, and seems to start some work on refactoring the core execution logic.

@Abhirup-99 Can you merge the most recent changes to this PR and retry the Stream Writer change I requested some time ago?

I still think the CopyToResponse method from fasthttp still doesn't copy the stream body in this case, but I think we can start experimenting with how to add this via fasthttp or Fiber if we merge the most recent updates to this PR.

@Abhirup-99 Abhirup-99 force-pushed the add-response branch 2 times, most recently from f8dce32 to efd9696 Compare November 3, 2025 19:10
Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 0

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (3)
client/core.go (1)

100-114: Per-request stream flag never applied

execFunc calls c.client.Do / DoRedirects with whatever streaming mode the transport already has, so a request-level SetStreamResponseBody(...) override is silently ignored. Before invoking the transport you need to inspect the request’s override, temporarily flip the transport’s StreamResponseBody flag, and restore it afterwards (ideally under a small mutex to avoid interleaving with other in-flight requests). Without that handoff the new per-request API is a no-op.

ctx_test.go (2)

3828-3832: Duplicate test case in Subdomains table; remove the second copy

The "offset equal len returns empty" case appears twice back-to-back. Drop the duplicate to avoid redundant execution.

@@
-        {
-            name:   "offset equal len returns empty",
-            host:   "john.doe.com",
-            offset: []int{3},
-            want:   []string{},
-        },

4312-4314: Duplicate '/test2' entry in testCases

The same test case is listed twice. Remove one to keep the table minimal and unambiguous.

@@
-        {url: "/test2", body: "type DefaultCtx struct", contentDisposition: "attachment"},
♻️ Duplicate comments (2)
client/response_test.go (2)

779-828: Read SSE progressively; avoid io.ReadAll for “incremental reads”

Use a bufio.Reader and consume event by event to validate streaming semantics.

@@
-        bodyStream := resp.BodyStream()
-        require.NotNil(t, bodyStream)
-
-        data, err := io.ReadAll(bodyStream)
-        require.NoError(t, err)
-
-        content := string(data)
-        require.Contains(t, content, "event 1")
-        require.Contains(t, content, "event 2")
-        require.Contains(t, content, "event 3")
-        require.Contains(t, content, "event 4")
-        require.Contains(t, content, "data: event")
-        require.Contains(t, content, "\n\n")
+        bodyStream := resp.BodyStream()
+        require.NotNil(t, bodyStream)
+        r := bufio.NewReader(bodyStream)
+        for i := 1; i <= 4; i++ {
+            line, err := r.ReadString('\n')
+            require.NoError(t, err)
+            require.Contains(t, line, fmt.Sprintf("data: event %d", i))
+            _, _ = r.ReadString('\n') // blank line
+        }

Add import if missing:

@@
-import (
+import (
+   "bufio"

714-726: Fix flakiness: copy Body() before closing the pooled response
The slice points to pooled memory; copy it before Close.

-        defer resp1.Close()
-        normalBody := resp1.Body()
+        defer resp1.Close()
+        normalBody := append([]byte(nil), resp1.Body()...)
🧹 Nitpick comments (3)
ctx_test.go (1)

5585-5596: Release acquired context to avoid pool leakage in tests

Add a deferred ReleaseCtx to mirror prevailing patterns and keep pools healthy under -race.

-        c := app.AcquireCtx(&fasthttp.RequestCtx{})
+        c := app.AcquireCtx(&fasthttp.RequestCtx{})
+        defer app.ReleaseCtx(c)
client/response.go (2)

92-101: Tighten BodyStream contract docs

Clarify lifetime/ordering and fallback to bytes.Reader for non-streaming responses.

-// BodyStream returns the response body as a stream reader.
-// Note: When using BodyStream(), the response body is not copied to memory,
-// so calling Body() afterwards may return an empty slice.
+// BodyStream returns an io.Reader for the response body.
+// When the underlying client/request enabled streaming, the reader yields data
+// directly from the connection without buffering; consume it before Close().
+// When streaming is disabled, BodyStream returns a bytes.Reader over Body().
+// Note: If you read via BodyStream while streaming is enabled, Body() may be empty.

155-160: Simplify Save: always copy from BodyStream()

BodyStream already returns a bytes.Reader when not streaming; drop the IsStreaming branches and copy once.

-        if r.IsStreaming() {
-            _, err = io.Copy(outFile, r.BodyStream())
-        } else {
-            _, err = io.Copy(outFile, bytes.NewReader(r.Body()))
-        }
+        _, err = io.Copy(outFile, r.BodyStream())
@@
-        var err error
-        if r.IsStreaming() {
-            _, err = io.Copy(p, r.BodyStream())
-        } else {
-            _, err = io.Copy(p, bytes.NewReader(r.Body()))
-        }
+        _, err := io.Copy(p, r.BodyStream())

[gaby suggested io.CopyBuffer; using plain io.Copy keeps it simple and idiomatic. Based on learnings]

Also applies to: 169-177

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 250b1c2 and efd9696.

📒 Files selected for processing (8)
  • client/client.go (1 hunks)
  • client/core.go (2 hunks)
  • client/core_test.go (1 hunks)
  • client/response.go (3 hunks)
  • client/response_test.go (2 hunks)
  • client/transport.go (4 hunks)
  • ctx_test.go (23 hunks)
  • docs/client/rest.md (1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
  • docs/client/rest.md
🧰 Additional context used
📓 Path-based instructions (1)
**/*.go

📄 CodeRabbit inference engine (AGENTS.md)

**/*.go: Apply formatting using gofumpt (Make target: format)
Optimize struct field alignment using betteralign (Make target: betteralign)
Modernize code using gopls modernize (Make target: modernize)

Files:

  • client/client.go
  • client/core_test.go
  • client/response.go
  • client/response_test.go
  • ctx_test.go
  • client/core.go
  • client/transport.go
🧠 Learnings (14)
📓 Common learnings
Learnt from: ReneWerner87
Repo: gofiber/fiber PR: 0
File: :0-0
Timestamp: 2024-12-01T10:28:36.011Z
Learning: Feature request #3224 has been created to add support for square bracket notation and comma-separated values in multipart form data in Fiber, while maintaining binary data transfer capabilities. This would bring parity with the existing form-urlencoded functionality.
📚 Learning: 2025-10-16T07:19:52.418Z
Learnt from: grivera64
Repo: gofiber/fiber PR: 3807
File: adapter_test.go:118-144
Timestamp: 2025-10-16T07:19:52.418Z
Learning: In the Fiber codebase, the linter does not allow `require` assertions from within HTTP handlers (including net/http-style handlers). Use `t.Fatalf`, `t.Errorf`, or similar `testing.T` methods for error handling inside handler functions instead.

Applied to files:

  • client/response.go
  • client/response_test.go
  • ctx_test.go
📚 Learning: 2025-09-28T17:39:26.644Z
Learnt from: arturmelanchyk
Repo: gofiber/fiber PR: 3768
File: client/request.go:162-166
Timestamp: 2025-09-28T17:39:26.644Z
Learning: In fasthttp-based code like gofiber, using utils.UnsafeString(key) instead of string(key) is acceptable when values are used immediately within the same call stack and not stored for later use, as it avoids allocations and aligns with fasthttp's zero-allocation design philosophy.

Applied to files:

  • client/response.go
📚 Learning: 2025-10-16T07:15:26.529Z
Learnt from: grivera64
Repo: gofiber/fiber PR: 3807
File: adapter_test.go:118-144
Timestamp: 2025-10-16T07:15:26.529Z
Learning: In Fiber v3, net/http handlers (http.Handler, http.HandlerFunc, or raw func(http.ResponseWriter, *http.Request)) can be passed directly to routing methods like app.Get(), app.Post(), etc. The framework automatically detects and wraps them internally via toFiberHandler/collectHandlers. The github.com/gofiber/fiber/v3/middleware/adaptor package is legacy and should not be suggested for tests or code using native net/http handler support.

Applied to files:

  • client/response.go
  • ctx_test.go
📚 Learning: 2024-11-29T12:37:27.581Z
Learnt from: efectn
Repo: gofiber/fiber PR: 3162
File: app_test.go:893-895
Timestamp: 2024-11-29T12:37:27.581Z
Learning: In the `Test_App_ShutdownWithContext` function in `app_test.go`, the `clientDone` channel is used to synchronize the client's request completion before proceeding, eliminating the need for additional `time.Sleep` calls.

Applied to files:

  • client/response_test.go
  • ctx_test.go
📚 Learning: 2024-07-26T21:00:12.902Z
Learnt from: sixcolors
Repo: gofiber/fiber PR: 2922
File: middleware/cors/utils.go:63-71
Timestamp: 2024-07-26T21:00:12.902Z
Learning: The project uses the testify/assert package for assertions in unit tests.

Applied to files:

  • ctx_test.go
📚 Learning: 2024-12-13T08:14:22.851Z
Learnt from: efectn
Repo: gofiber/fiber PR: 3162
File: hooks_test.go:228-228
Timestamp: 2024-12-13T08:14:22.851Z
Learning: In Go test files, prefer using the `require` methods from the `testify` package for assertions instead of manual comparisons and calls to `t.Fatal` or `t.Fatalf`.

Applied to files:

  • ctx_test.go
📚 Learning: 2024-11-15T07:56:21.623Z
Learnt from: ReneWerner87
Repo: gofiber/fiber PR: 3161
File: app.go:923-932
Timestamp: 2024-11-15T07:56:21.623Z
Learning: In the Fiber framework, breaking changes are acceptable when moving from version 2 to version 3, including modifications to method signatures such as in the `Test` method in `app.go`.

Applied to files:

  • ctx_test.go
📚 Learning: 2024-10-08T19:06:06.583Z
Learnt from: sixcolors
Repo: gofiber/fiber PR: 3016
File: middleware/session/store.go:164-167
Timestamp: 2024-10-08T19:06:06.583Z
Learning: Unit tests in this project use testify require.

Applied to files:

  • ctx_test.go
📚 Learning: 2025-10-22T13:48:12.485Z
Learnt from: CR
Repo: gofiber/fiber PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-10-22T13:48:12.485Z
Learning: Run tests with gotestsum (Make target: test)

Applied to files:

  • ctx_test.go
📚 Learning: 2024-11-10T23:44:13.704Z
Learnt from: gaby
Repo: gofiber/fiber PR: 3193
File: middleware/adaptor/adaptor.go:111-111
Timestamp: 2024-11-10T23:44:13.704Z
Learning: In the `middleware/adaptor/adaptor.go` file of the Fiber framework, when updating context handling, replacing `c.Context()` with `c.RequestCtx()` is appropriate to access the `fasthttp.RequestCtx`.

Applied to files:

  • ctx_test.go
📚 Learning: 2024-09-25T15:57:10.221Z
Learnt from: sixcolors
Repo: gofiber/fiber PR: 3016
File: middleware/csrf/csrf_test.go:188-193
Timestamp: 2024-09-25T15:57:10.221Z
Learning: In the Fiber framework tests, using `ctx.Response.Header.Cookie` may not be suitable for parsing cookies from the response header, as it requires a `*Cookie` and fills it rather than returning a string value; thus, manual parsing of the `Set-Cookie` header may be necessary.

Applied to files:

  • ctx_test.go
📚 Learning: 2024-09-25T17:05:06.991Z
Learnt from: sixcolors
Repo: gofiber/fiber PR: 3016
File: middleware/session/middleware_test.go:190-191
Timestamp: 2024-09-25T17:05:06.991Z
Learning: When testing session `IdleTimeout` expiration, it's acceptable to use `time.Sleep` to simulate the passage of time in tests.

Applied to files:

  • ctx_test.go
📚 Learning: 2024-10-08T19:06:06.583Z
Learnt from: sixcolors
Repo: gofiber/fiber PR: 3051
File: middleware/session/session.go:215-216
Timestamp: 2024-10-08T19:06:06.583Z
Learning: Parallel tests for `Session.Save` already exist in the `middleware/session/session_test.go` file, specifically in the `Test_Session_Save` and `Test_Session_Save_Expiration` functions.

Applied to files:

  • ctx_test.go
🧬 Code graph analysis (3)
client/response_test.go (2)
client/client.go (1)
  • New (752-757)
client/response.go (1)
  • Response (19-25)
ctx_test.go (3)
req_interface_gen.go (1)
  • Req (12-188)
req.go (1)
  • Params (619-625)
app.go (1)
  • TestConfig (1090-1101)
client/core.go (1)
client/response.go (1)
  • ReleaseResponse (232-235)
🔇 Additional comments (1)
ctx_test.go (1)

676-701: Toolchain compatibility verified — no action required

The go.mod file correctly specifies Go 1.25.0, which exceeds the minimum Go 1.23 requirement for strings.SplitSeq and range-over-iterators. The code at the flagged lines (ctx_test.go 676-701 and 912-936) is compatible with the configured Go version.

@Abhirup-99
Copy link
Author

@gaby @griver please review now.

Copy link
Member

@grivera64 grivera64 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There are several test cases that don't use streaming methods (c.SendStream() or c.SendStreamWriter()).

It's best to update those tests to ensure that we aren't just skipping the streaming logic since streaming mode isn't active.

@Abhirup-99
Copy link
Author

@grivera64 done

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 0

🧹 Nitpick comments (1)
client/response_test.go (1)

720-750: Consider defensive copy to prevent potential flakiness.

While the current code is likely safe (comparison happens before defers execute), past review feedback suggests making a copy of normalBody to guard against the underlying memory being reused if response pooling behavior changes:

normalBody := append([]byte(nil), resp1.Body()...)

This ensures the comparison at line 750 uses a stable copy even if internal fasthttp pooling behavior changes in the future.

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 79e9380 and 60a12fb.

📒 Files selected for processing (8)
  • client/client.go (1 hunks)
  • client/core.go (2 hunks)
  • client/core_test.go (1 hunks)
  • client/response.go (3 hunks)
  • client/response_test.go (2 hunks)
  • client/transport.go (4 hunks)
  • ctx_test.go (11 hunks)
  • docs/client/rest.md (1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
  • ctx_test.go
  • client/response.go
🧰 Additional context used
📓 Path-based instructions (2)
**/*.go

📄 CodeRabbit inference engine (AGENTS.md)

**/*.go: Apply formatting using gofumpt (Make target: format)
Optimize struct field alignment using betteralign (Make target: betteralign)
Modernize code using gopls modernize (Make target: modernize)

Files:

  • client/core_test.go
  • client/core.go
  • client/transport.go
  • client/response_test.go
  • client/client.go
docs/**

📄 CodeRabbit inference engine (.github/copilot-instructions.md)

Review and update the contents of the docs folder if necessary when modifying code

Files:

  • docs/client/rest.md
🧠 Learnings (5)
📚 Learning: 2025-10-16T07:19:52.418Z
Learnt from: grivera64
Repo: gofiber/fiber PR: 3807
File: adapter_test.go:118-144
Timestamp: 2025-10-16T07:19:52.418Z
Learning: In the Fiber codebase, the linter does not allow `require` assertions from within HTTP handlers (including net/http-style handlers). Use `t.Fatalf`, `t.Errorf`, or similar `testing.T` methods for error handling inside handler functions instead.

Applied to files:

  • client/response_test.go
📚 Learning: 2024-11-29T12:37:27.581Z
Learnt from: efectn
Repo: gofiber/fiber PR: 3162
File: app_test.go:893-895
Timestamp: 2024-11-29T12:37:27.581Z
Learning: In the `Test_App_ShutdownWithContext` function in `app_test.go`, the `clientDone` channel is used to synchronize the client's request completion before proceeding, eliminating the need for additional `time.Sleep` calls.

Applied to files:

  • client/response_test.go
📚 Learning: 2024-09-25T16:18:34.719Z
Learnt from: sixcolors
Repo: gofiber/fiber PR: 3016
File: middleware/session/config.go:122-122
Timestamp: 2024-09-25T16:18:34.719Z
Learning: In `DefaultErrorHandler(c *fiber.Ctx, err error)`, since `c` is a pointer to an interface, we need to dereference `*c` when calling interface methods like `SendStatus`.

Applied to files:

  • client/response_test.go
📚 Learning: 2024-10-08T19:06:06.583Z
Learnt from: sixcolors
Repo: gofiber/fiber PR: 3016
File: middleware/session/store.go:164-167
Timestamp: 2024-10-08T19:06:06.583Z
Learning: Unit tests in this project use testify require.

Applied to files:

  • client/response_test.go
📚 Learning: 2024-07-26T21:00:12.902Z
Learnt from: sixcolors
Repo: gofiber/fiber PR: 2922
File: middleware/cors/utils.go:63-71
Timestamp: 2024-07-26T21:00:12.902Z
Learning: The project uses the testify/assert package for assertions in unit tests.

Applied to files:

  • client/response_test.go
🧬 Code graph analysis (2)
client/core.go (1)
client/response.go (1)
  • ReleaseResponse (232-235)
client/response_test.go (2)
client/client.go (1)
  • New (752-757)
client/response.go (1)
  • Response (19-25)
🔇 Additional comments (16)
docs/client/rest.md (1)

409-423: LGTM! Documentation is clear and complete.

The streaming API documentation correctly describes:

  • The behavior and use cases for response body streaming
  • How to enable/disable streaming at the client level
  • The relationship with BodyStream() and Body() methods
client/transport.go (4)

30-31: LGTM! Interface additions are well-designed.

The streaming control methods are properly added to the httpClientTransport interface, enabling consistent behavior across all transport implementations.


80-86: LGTM! Standard transport streaming implementation is correct.

The implementation correctly delegates to the underlying fasthttp.Client.StreamResponseBody field.


134-140: LGTM! Host client transport streaming implementation is correct.

The implementation correctly delegates to the underlying fasthttp.HostClient.StreamResponseBody field.


200-221: LGTM! Load balancer transport handles nested clients correctly.

The implementation:

  • Returns false when no clients exist (safe default)
  • Walks nested clients to find the first streaming setting (getter)
  • Propagates settings to all nested clients (setter)

This correctly handles the load balancer's composite structure.

client/core_test.go (1)

386-391: LGTM! Test transport stub methods are appropriate.

The streaming stubs correctly implement the interface for the test mock, returning safe defaults that won't affect existing test behavior.

client/client.go (1)

529-541: LGTM! Client-level streaming API is correctly implemented.

The methods properly delegate to the transport layer, maintaining clean separation of concerns. The chainable setter follows established patterns in the codebase.

client/core.go (3)

89-93: LGTM! Nil check prevents double-release.

The guard correctly handles the case where respv is transferred to resp.RawResponse and set to nil at line 127, preventing a double-release in the defer.


96-98: LGTM! BodyStream propagation is correct.

Properly forwards request body streams (e.g., for streaming uploads) to the underlying fasthttp request with the correct content length.


125-130: LGTM! RawResponse swap and resource management is correct.

The implementation:

  • Saves the original RawResponse reference
  • Transfers ownership of respv to resp.RawResponse
  • Sets respv = nil to prevent the defer from releasing the now-transferred response
  • Releases the original RawResponse if present

This ensures proper resource cleanup during streaming without leaks or double-frees.

client/response_test.go (6)

4-4: LGTM! Import additions support new test functionality.

All new imports (bufio, encoding/json, fmt, strings, time) are necessary for the comprehensive streaming tests.

Also applies to: 7-7, 9-9, 13-13, 15-15


546-619: LGTM! Streaming Save() tests are comprehensive.

Both tests correctly:

  • Use SendStreamWriter to ensure actual streaming behavior (addresses past review feedback)
  • Exercise the streaming Save() path with proper error handling
  • Verify that writers are not closed by Save() (line 584)
  • Test error propagation during streaming writes (lines 615-618)

622-654: LGTM! Test helper types are well-designed.

  • testClosableBuffer correctly tracks closure state
  • testErrorWriter accurately simulates write failures after a threshold

Both helpers enable precise testing of streaming error paths.


656-1083: LGTM! Comprehensive BodyStream test coverage.

All 10 subtests correctly:

  • Use SendStream or SendStreamWriter for actual streaming behavior (addresses all past grivera64 comments)
  • Enable streaming via SetStreamResponseBody(true)
  • Cover diverse scenarios: basic streaming, large responses, chunked data, SSE, JSON streaming, interruptions, and stream identity
  • Follow testify/require assertion patterns per project standards

1085-1169: LGTM! Fallback tests verify non-streaming behavior.

Tests correctly verify:

  • IsStreaming() returns false when streaming is disabled
  • BodyStream() returns a working bytes.Reader fallback
  • Both empty and large responses work correctly in non-streaming mode

1171-1244: LGTM! IsStreaming tests validate streaming state detection.

Tests comprehensively verify:

  • Streaming enabled/disabled states
  • BodyStream() works in both modes
  • Consistent behavior across streaming and non-streaming clients

@Abhirup-99 Abhirup-99 force-pushed the add-response branch 2 times, most recently from 45b8314 to 6751f82 Compare November 4, 2025 17:06
Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 0

♻️ Duplicate comments (1)
client/response_test.go (1)

720-751: Copy Body() bytes before closing response to avoid flaky test.

Line 743 retains a reference to resp1.Body() which points to the internal buffer. After resp1.Close() (deferred at line 742), that memory can be reused by the pool, making the comparison at line 750 potentially flaky.

Apply this diff to fix:

 		client1 := New().SetDial(server.dial())
 		resp1, err := client1.Get("http://example.com/stream")
 		require.NoError(t, err)
 		defer resp1.Close()
-		normalBody := resp1.Body()
+		normalBody := append([]byte(nil), resp1.Body()...)
 		client2 := New().SetDial(server.dial()).SetStreamResponseBody(true)
🧹 Nitpick comments (2)
client/response.go (2)

92-101: Clarify BodyStream contract and consumption guarantees in documentation.

The current documentation mentions that Body() may return empty after streaming, but doesn't explicitly state when the reader must be consumed or what happens if the response is closed mid-stream.

Apply this diff to improve the documentation:

-// BodyStream returns the response body as a stream reader.
-// Note: When using BodyStream(), the response body is not copied to memory,
-// so calling Body() afterwards may return an empty slice.
+// BodyStream returns an io.Reader for the response body.
+// If streaming was enabled (via Request.SetStreamResponseBody or Client.SetStreamResponseBody),
+// the reader yields data directly from the connection without buffering.
+//
+// Important:
+// - Consume the reader before calling Response.Close().
+// - When streaming is enabled, Body() and helpers like String/JSON/XML may see an empty body.
+// - Prefer io.Copy with BodyStream() for large responses.
 func (r *Response) BodyStream() io.Reader {

155-179: Simplify Save() by removing redundant IsStreaming checks.

Since BodyStream() already handles both streaming and non-streaming cases (returning the stream when available, or a bytes.Reader otherwise), the explicit IsStreaming() checks are redundant and add unnecessary branching.

Apply this diff to simplify both save paths:

 	case string:
 		// ... directory creation code ...
 
-		if r.IsStreaming() {
-			_, err = io.Copy(outFile, r.BodyStream())
-		} else {
-			_, err = io.Copy(outFile, bytes.NewReader(r.Body()))
-		}
+		_, err = io.Copy(outFile, r.BodyStream())
 
 		if err != nil {
 			return fmt.Errorf("failed to write response body to file: %w", err)
 		}
 
 		return nil
 
 	case io.Writer:
-		var err error
-		if r.IsStreaming() {
-			_, err = io.Copy(p, r.BodyStream())
-		} else {
-			_, err = io.Copy(p, bytes.NewReader(r.Body()))
-		}
-
-		if err != nil {
+		if _, err := io.Copy(p, r.BodyStream()); err != nil {
 			return fmt.Errorf("failed to write response body to writer: %w", err)
 		}
 
 		return nil
📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 60a12fb and 6751f82.

📒 Files selected for processing (8)
  • client/client.go (1 hunks)
  • client/core.go (2 hunks)
  • client/core_test.go (1 hunks)
  • client/response.go (3 hunks)
  • client/response_test.go (2 hunks)
  • client/transport.go (4 hunks)
  • ctx_test.go (2 hunks)
  • docs/client/rest.md (1 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
  • client/client.go
  • client/core_test.go
  • docs/client/rest.md
🧰 Additional context used
📓 Path-based instructions (1)
**/*.go

📄 CodeRabbit inference engine (AGENTS.md)

**/*.go: Apply formatting using gofumpt (Make target: format)
Optimize struct field alignment using betteralign (Make target: betteralign)
Modernize code using gopls modernize (Make target: modernize)

Files:

  • client/core.go
  • ctx_test.go
  • client/response.go
  • client/transport.go
  • client/response_test.go
🧠 Learnings (8)
📚 Learning: 2024-11-29T12:37:27.581Z
Learnt from: efectn
Repo: gofiber/fiber PR: 3162
File: app_test.go:893-895
Timestamp: 2024-11-29T12:37:27.581Z
Learning: In the `Test_App_ShutdownWithContext` function in `app_test.go`, the `clientDone` channel is used to synchronize the client's request completion before proceeding, eliminating the need for additional `time.Sleep` calls.

Applied to files:

  • ctx_test.go
  • client/response_test.go
📚 Learning: 2024-09-25T17:05:06.991Z
Learnt from: sixcolors
Repo: gofiber/fiber PR: 3016
File: middleware/session/middleware_test.go:190-191
Timestamp: 2024-09-25T17:05:06.991Z
Learning: When testing session `IdleTimeout` expiration, it's acceptable to use `time.Sleep` to simulate the passage of time in tests.

Applied to files:

  • ctx_test.go
📚 Learning: 2025-10-16T07:19:52.418Z
Learnt from: grivera64
Repo: gofiber/fiber PR: 3807
File: adapter_test.go:118-144
Timestamp: 2025-10-16T07:19:52.418Z
Learning: In the Fiber codebase, the linter does not allow `require` assertions from within HTTP handlers (including net/http-style handlers). Use `t.Fatalf`, `t.Errorf`, or similar `testing.T` methods for error handling inside handler functions instead.

Applied to files:

  • client/response.go
  • client/response_test.go
📚 Learning: 2025-09-28T17:39:26.644Z
Learnt from: arturmelanchyk
Repo: gofiber/fiber PR: 3768
File: client/request.go:162-166
Timestamp: 2025-09-28T17:39:26.644Z
Learning: In fasthttp-based code like gofiber, using utils.UnsafeString(key) instead of string(key) is acceptable when values are used immediately within the same call stack and not stored for later use, as it avoids allocations and aligns with fasthttp's zero-allocation design philosophy.

Applied to files:

  • client/response.go
📚 Learning: 2025-10-16T07:15:26.529Z
Learnt from: grivera64
Repo: gofiber/fiber PR: 3807
File: adapter_test.go:118-144
Timestamp: 2025-10-16T07:15:26.529Z
Learning: In Fiber v3, net/http handlers (http.Handler, http.HandlerFunc, or raw func(http.ResponseWriter, *http.Request)) can be passed directly to routing methods like app.Get(), app.Post(), etc. The framework automatically detects and wraps them internally via toFiberHandler/collectHandlers. The github.com/gofiber/fiber/v3/middleware/adaptor package is legacy and should not be suggested for tests or code using native net/http handler support.

Applied to files:

  • client/response.go
📚 Learning: 2024-09-25T16:18:34.719Z
Learnt from: sixcolors
Repo: gofiber/fiber PR: 3016
File: middleware/session/config.go:122-122
Timestamp: 2024-09-25T16:18:34.719Z
Learning: In `DefaultErrorHandler(c *fiber.Ctx, err error)`, since `c` is a pointer to an interface, we need to dereference `*c` when calling interface methods like `SendStatus`.

Applied to files:

  • client/response_test.go
📚 Learning: 2024-10-08T19:06:06.583Z
Learnt from: sixcolors
Repo: gofiber/fiber PR: 3016
File: middleware/session/store.go:164-167
Timestamp: 2024-10-08T19:06:06.583Z
Learning: Unit tests in this project use testify require.

Applied to files:

  • client/response_test.go
📚 Learning: 2024-07-26T21:00:12.902Z
Learnt from: sixcolors
Repo: gofiber/fiber PR: 2922
File: middleware/cors/utils.go:63-71
Timestamp: 2024-07-26T21:00:12.902Z
Learning: The project uses the testify/assert package for assertions in unit tests.

Applied to files:

  • client/response_test.go
🧬 Code graph analysis (3)
client/core.go (1)
client/response.go (1)
  • ReleaseResponse (232-235)
ctx_test.go (1)
app.go (1)
  • TestConfig (1090-1101)
client/response_test.go (2)
client/client.go (1)
  • New (752-757)
client/response.go (1)
  • Response (19-25)
🔇 Additional comments (10)
ctx_test.go (2)

3778-3778: Good addition: Timeout prevents flaky file I/O tests.

Adding a 5-second timeout for this file upload test is appropriate since file I/O operations can be slow in various environments (CI/CD, containers, slower disks). The default 1-second timeout might be insufficient and lead to flaky test failures.


3820-3820: Good addition: Consistent timeout for storage operations.

This timeout addition is consistent with the change in Test_Ctx_SaveFile (line 3778) and appropriate for storage operations, which may involve additional overhead. This helps prevent flaky tests in various environments.

client/response_test.go (2)

546-619: LGTM! Comprehensive streaming Save() test coverage.

The new test cases properly verify:

  • Save() doesn't close writers it didn't create (testClosableBuffer helper)
  • Error handling during streaming copy (testErrorWriter helper)
  • Both test helpers are well-designed and appropriate for their purpose

656-1083: Excellent streaming test coverage.

The test suite thoroughly exercises streaming behavior across multiple scenarios:

  • Basic streaming with SendStream
  • Large responses with incremental reads
  • Chunked streaming with SendStreamWriter and delays
  • SSE-style progressive delivery
  • Connection interruption handling
  • Stream object identity verification

All tests properly use SendStream() or SendStreamWriter() to ensure true streaming behavior on the server side, addressing previous review concerns.

client/transport.go (3)

30-31: LGTM! Clean interface extension for streaming control.

The new StreamResponseBody() and SetStreamResponseBody(enable bool) methods properly extend the httpClientTransport interface to support response streaming configuration.


80-86: LGTM! Straightforward streaming delegation for standard transports.

Both standardClientTransport and hostClientTransport correctly delegate to the underlying fasthttp client's StreamResponseBody field.

Also applies to: 134-140


200-221: LGTM! Proper nested-client traversal for load balancer streaming control.

The lbClientTransport implementation correctly:

  • Returns false when no clients are configured (line 201-203)
  • Walks nested balancing clients to find the first StreamResponseBody setting (lines 206-213)
  • Propagates the setting to all nested clients via forEachHostClient (lines 217-220)

This mirrors the pattern used for TLSConfig and properly handles the complexity of nested load balancers.

client/core.go (3)

89-93: LGTM! Proper guard against double-release.

The nil-check in the defer ensures respv is only released once, preventing double-release after the response swap at line 127.


96-98: LGTM! Correct BodyStream propagation for streaming requests.

When the request contains a BodyStream, it's properly forwarded to the fasthttp request with the correct ContentLength. This enables streaming request bodies (e.g., file uploads, progressive data).


125-130: LGTM! Safe RawResponse swap with proper cleanup.

The implementation correctly:

  • Preserves the original RawResponse before swapping (line 125)
  • Assigns the new response (line 126)
  • Clears respv to prevent double-release (line 127)
  • Releases the original if present (lines 128-130)

This ensures proper resource management when responses are reused or pooled.

Copy link
Member

@grivera64 grivera64 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM 🚀 Let's wait for the workflow approval before we merge these changes.

@Abhirup-99
Copy link
Author

@gaby can you check this out?

@grivera64
Copy link
Member

@Abhirup-99 Can you take a look at the coverage report from the codecov workflow? I think a few of the low coverage lines can be covered with a few simple unit tests.

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 0

🧹 Nitpick comments (3)
client/response.go (1)

155-176: Drop the duplicated branches in Save and rely on BodyStream’s fallback

BodyStream() already returns the raw fasthttp stream when streaming is enabled and a fresh *bytes.Reader when it isn’t, so you can skip the IsStreaming() checks and call it unconditionally. That trims duplicate code and avoids the second RawResponse.Body() call.

-		if r.IsStreaming() {
-			_, err = io.Copy(outFile, r.BodyStream())
-		} else {
-			_, err = io.Copy(outFile, bytes.NewReader(r.Body()))
-		}
+		_, err = io.Copy(outFile, r.BodyStream())
@@
-		var err error
-		if r.IsStreaming() {
-			_, err = io.Copy(p, r.BodyStream())
-		} else {
-			_, err = io.Copy(p, bytes.NewReader(r.Body()))
-		}
+		var err error
+		_, err = io.Copy(p, r.BodyStream())
client/transport_test.go (2)

414-421: Extend test to verify enable/disable with empty clients.

The "empty clients" subtest only checks the default value. Consider adding assertions to verify that calling SetStreamResponseBody(true) and SetStreamResponseBody(false) with an empty client list doesn't panic and returns the expected value.

Apply this diff to make the test more complete:

 	t.Run("empty clients", func(t *testing.T) {
 		t.Parallel()
 		lbClient := &fasthttp.LBClient{
 			Clients: []fasthttp.BalancingClient{},
 		}
 		transport := newLBClientTransport(lbClient)
 		require.False(t, transport.StreamResponseBody())
+
+		// Verify enable/disable doesn't panic with empty clients
+		transport.SetStreamResponseBody(true)
+		require.True(t, transport.StreamResponseBody())
+		transport.SetStreamResponseBody(false)
+		require.False(t, transport.StreamResponseBody())
 	})

467-488: Add assertions for transport return value for consistency.

Unlike other subtests, the "nested lb client" test doesn't verify transport.StreamResponseBody() return value after calling SetStreamResponseBody. For consistency with the other subtests (e.g., "single host client", "multiple host clients"), add assertions to check the transport's return value.

Apply this diff to improve consistency:

 	t.Run("nested lb client", func(t *testing.T) {
 		t.Parallel()
 		hostClient1 := &fasthttp.HostClient{Addr: "example1.com:80"}
 		hostClient2 := &fasthttp.HostClient{Addr: "example2.com:80"}
 		nestedLB := &fasthttp.LBClient{
 			Clients: []fasthttp.BalancingClient{hostClient1, hostClient2},
 		}
 		lbClient := &fasthttp.LBClient{
 			Clients: []fasthttp.BalancingClient{&lbBalancingClient{client: nestedLB}},
 		}
 		transport := newLBClientTransport(lbClient)
 
 		// Enable streaming on nested clients
 		transport.SetStreamResponseBody(true)
+		require.True(t, transport.StreamResponseBody())
 		require.True(t, hostClient1.StreamResponseBody)
 		require.True(t, hostClient2.StreamResponseBody)
 
 		// Disable streaming on nested clients
 		transport.SetStreamResponseBody(false)
+		require.False(t, transport.StreamResponseBody())
 		require.False(t, hostClient1.StreamResponseBody)
 		require.False(t, hostClient2.StreamResponseBody)
 	})
📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 6751f82 and c4c1dc7.

📒 Files selected for processing (10)
  • client/client.go (1 hunks)
  • client/client_test.go (1 hunks)
  • client/core.go (2 hunks)
  • client/core_test.go (1 hunks)
  • client/response.go (3 hunks)
  • client/response_test.go (2 hunks)
  • client/transport.go (4 hunks)
  • client/transport_test.go (1 hunks)
  • ctx_test.go (2 hunks)
  • docs/client/rest.md (1 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
  • ctx_test.go
  • client/client.go
  • client/core.go
  • docs/client/rest.md
🧰 Additional context used
📓 Path-based instructions (1)
**/*.go

📄 CodeRabbit inference engine (AGENTS.md)

**/*.go: Apply formatting using gofumpt (Make target: format)
Optimize struct field alignment using betteralign (Make target: betteralign)
Modernize code using gopls modernize (Make target: modernize)

Files:

  • client/response.go
  • client/transport_test.go
  • client/client_test.go
  • client/core_test.go
  • client/transport.go
  • client/response_test.go
🧠 Learnings (9)
📚 Learning: 2025-10-16T07:19:52.418Z
Learnt from: grivera64
Repo: gofiber/fiber PR: 3807
File: adapter_test.go:118-144
Timestamp: 2025-10-16T07:19:52.418Z
Learning: In the Fiber codebase, the linter does not allow `require` assertions from within HTTP handlers (including net/http-style handlers). Use `t.Fatalf`, `t.Errorf`, or similar `testing.T` methods for error handling inside handler functions instead.

Applied to files:

  • client/response.go
  • client/client_test.go
  • client/response_test.go
📚 Learning: 2025-09-28T17:39:26.644Z
Learnt from: arturmelanchyk
Repo: gofiber/fiber PR: 3768
File: client/request.go:162-166
Timestamp: 2025-09-28T17:39:26.644Z
Learning: In fasthttp-based code like gofiber, using utils.UnsafeString(key) instead of string(key) is acceptable when values are used immediately within the same call stack and not stored for later use, as it avoids allocations and aligns with fasthttp's zero-allocation design philosophy.

Applied to files:

  • client/response.go
📚 Learning: 2025-10-16T07:15:26.529Z
Learnt from: grivera64
Repo: gofiber/fiber PR: 3807
File: adapter_test.go:118-144
Timestamp: 2025-10-16T07:15:26.529Z
Learning: In Fiber v3, net/http handlers (http.Handler, http.HandlerFunc, or raw func(http.ResponseWriter, *http.Request)) can be passed directly to routing methods like app.Get(), app.Post(), etc. The framework automatically detects and wraps them internally via toFiberHandler/collectHandlers. The github.com/gofiber/fiber/v3/middleware/adaptor package is legacy and should not be suggested for tests or code using native net/http handler support.

Applied to files:

  • client/response.go
📚 Learning: 2024-12-13T08:14:22.851Z
Learnt from: efectn
Repo: gofiber/fiber PR: 3162
File: hooks_test.go:228-228
Timestamp: 2024-12-13T08:14:22.851Z
Learning: In Go test files, prefer using the `require` methods from the `testify` package for assertions instead of manual comparisons and calls to `t.Fatal` or `t.Fatalf`.

Applied to files:

  • client/client_test.go
📚 Learning: 2024-11-29T12:37:27.581Z
Learnt from: efectn
Repo: gofiber/fiber PR: 3162
File: app_test.go:893-895
Timestamp: 2024-11-29T12:37:27.581Z
Learning: In the `Test_App_ShutdownWithContext` function in `app_test.go`, the `clientDone` channel is used to synchronize the client's request completion before proceeding, eliminating the need for additional `time.Sleep` calls.

Applied to files:

  • client/client_test.go
  • client/response_test.go
📚 Learning: 2024-09-25T16:18:34.719Z
Learnt from: sixcolors
Repo: gofiber/fiber PR: 3016
File: middleware/session/config.go:122-122
Timestamp: 2024-09-25T16:18:34.719Z
Learning: In `DefaultErrorHandler(c *fiber.Ctx, err error)`, since `c` is a pointer to an interface, we need to dereference `*c` when calling interface methods like `SendStatus`.

Applied to files:

  • client/client_test.go
  • client/response_test.go
📚 Learning: 2024-11-10T23:44:13.704Z
Learnt from: gaby
Repo: gofiber/fiber PR: 3193
File: middleware/adaptor/adaptor.go:111-111
Timestamp: 2024-11-10T23:44:13.704Z
Learning: In the `middleware/adaptor/adaptor.go` file of the Fiber framework, when updating context handling, replacing `c.Context()` with `c.RequestCtx()` is appropriate to access the `fasthttp.RequestCtx`.

Applied to files:

  • client/client_test.go
📚 Learning: 2024-10-02T23:03:31.727Z
Learnt from: sixcolors
Repo: gofiber/fiber PR: 3016
File: middleware/session/store.go:164-167
Timestamp: 2024-10-02T23:03:31.727Z
Learning: Unit tests in this project use testify require.

Applied to files:

  • client/response_test.go
📚 Learning: 2024-10-08T19:06:06.583Z
Learnt from: sixcolors
Repo: gofiber/fiber PR: 2922
File: middleware/cors/utils.go:63-71
Timestamp: 2024-10-08T19:06:06.583Z
Learning: The project uses the testify/assert package for assertions in unit tests.

Applied to files:

  • client/response_test.go
🧬 Code graph analysis (2)
client/client_test.go (1)
client/client.go (3)
  • New (752-757)
  • NewWithHostClient (768-773)
  • NewWithLBClient (776-781)
client/response_test.go (3)
client/client.go (2)
  • Get (831-833)
  • New (752-757)
ctx_interface_gen.go (1)
  • Ctx (18-428)
client/response.go (3)
  • Response (19-25)
  • AcquireResponse (222-228)
  • ReleaseResponse (232-235)
🔇 Additional comments (2)
client/transport_test.go (2)

350-378: LGTM! Comprehensive streaming toggle tests.

The test thoroughly covers default state, enabling, and disabling streaming for StandardClientTransport, verifying both the transport wrapper and the underlying fasthttp.Client reflect the changes correctly.


380-409: LGTM! Consistent test coverage for HostClientTransport.

The test structure mirrors the StandardClientTransport tests and correctly verifies streaming state propagation to the underlying HostClient.

@Abhirup-99
Copy link
Author

@grivera64 can you check now?

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 0

🧹 Nitpick comments (1)
client/response.go (1)

92-106: Consider clarifying BodyStream() documentation.

The implementation is correct, but the documentation could be more explicit about the consumption contract and relationship with Body(). Consider expanding the doc to note:

  • The reader must be consumed before closing the Response
  • When streaming is enabled, Body() may return empty after consuming BodyStream()
  • For non-streaming responses, BodyStream() returns a bytes.Reader over the buffered body

Example enhancement:

-// BodyStream returns the response body as a stream reader.
-// Note: When using BodyStream(), the response body is not copied to memory,
-// so calling Body() afterwards may return an empty slice.
+// BodyStream returns an io.Reader for the response body.
+// When streaming is enabled (via SetStreamResponseBody), the reader yields data
+// directly from the connection without buffering. Otherwise, it returns a bytes.Reader
+// over the buffered body.
+//
+// Important: consume the reader before calling Response.Close(). When streaming is
+// enabled, Body() may return empty after the stream has been read.
 func (r *Response) BodyStream() io.Reader {
📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between c4c1dc7 and 402be56.

📒 Files selected for processing (10)
  • client/client.go (1 hunks)
  • client/client_test.go (1 hunks)
  • client/core.go (2 hunks)
  • client/core_test.go (1 hunks)
  • client/response.go (3 hunks)
  • client/response_test.go (2 hunks)
  • client/transport.go (4 hunks)
  • client/transport_test.go (1 hunks)
  • ctx_test.go (2 hunks)
  • docs/client/rest.md (1 hunks)
🚧 Files skipped from review as they are similar to previous changes (5)
  • client/client_test.go
  • client/transport_test.go
  • client/core_test.go
  • docs/client/rest.md
  • ctx_test.go
🧰 Additional context used
📓 Path-based instructions (1)
**/*.go

📄 CodeRabbit inference engine (AGENTS.md)

**/*.go: Apply formatting using gofumpt (Make target: format)
Optimize struct field alignment using betteralign (Make target: betteralign)
Modernize code using gopls modernize (Make target: modernize)

Files:

  • client/core.go
  • client/client.go
  • client/response_test.go
  • client/transport.go
  • client/response.go
🧠 Learnings (7)
📚 Learning: 2025-10-16T07:19:52.418Z
Learnt from: grivera64
Repo: gofiber/fiber PR: 3807
File: adapter_test.go:118-144
Timestamp: 2025-10-16T07:19:52.418Z
Learning: In the Fiber codebase, the linter does not allow `require` assertions from within HTTP handlers (including net/http-style handlers). Use `t.Fatalf`, `t.Errorf`, or similar `testing.T` methods for error handling inside handler functions instead.

Applied to files:

  • client/response_test.go
  • client/response.go
📚 Learning: 2024-11-29T12:37:27.581Z
Learnt from: efectn
Repo: gofiber/fiber PR: 3162
File: app_test.go:893-895
Timestamp: 2024-11-29T12:37:27.581Z
Learning: In the `Test_App_ShutdownWithContext` function in `app_test.go`, the `clientDone` channel is used to synchronize the client's request completion before proceeding, eliminating the need for additional `time.Sleep` calls.

Applied to files:

  • client/response_test.go
📚 Learning: 2024-09-25T16:18:34.719Z
Learnt from: sixcolors
Repo: gofiber/fiber PR: 3016
File: middleware/session/config.go:122-122
Timestamp: 2024-09-25T16:18:34.719Z
Learning: In `DefaultErrorHandler(c *fiber.Ctx, err error)`, since `c` is a pointer to an interface, we need to dereference `*c` when calling interface methods like `SendStatus`.

Applied to files:

  • client/response_test.go
📚 Learning: 2024-10-02T23:03:31.727Z
Learnt from: sixcolors
Repo: gofiber/fiber PR: 3016
File: middleware/session/store.go:164-167
Timestamp: 2024-10-02T23:03:31.727Z
Learning: Unit tests in this project use testify require.

Applied to files:

  • client/response_test.go
📚 Learning: 2024-10-08T19:06:06.583Z
Learnt from: sixcolors
Repo: gofiber/fiber PR: 2922
File: middleware/cors/utils.go:63-71
Timestamp: 2024-10-08T19:06:06.583Z
Learning: The project uses the testify/assert package for assertions in unit tests.

Applied to files:

  • client/response_test.go
📚 Learning: 2025-09-28T17:39:26.644Z
Learnt from: arturmelanchyk
Repo: gofiber/fiber PR: 3768
File: client/request.go:162-166
Timestamp: 2025-09-28T17:39:26.644Z
Learning: In fasthttp-based code like gofiber, using utils.UnsafeString(key) instead of string(key) is acceptable when values are used immediately within the same call stack and not stored for later use, as it avoids allocations and aligns with fasthttp's zero-allocation design philosophy.

Applied to files:

  • client/response.go
📚 Learning: 2025-10-16T07:15:26.529Z
Learnt from: grivera64
Repo: gofiber/fiber PR: 3807
File: adapter_test.go:118-144
Timestamp: 2025-10-16T07:15:26.529Z
Learning: In Fiber v3, net/http handlers (http.Handler, http.HandlerFunc, or raw func(http.ResponseWriter, *http.Request)) can be passed directly to routing methods like app.Get(), app.Post(), etc. The framework automatically detects and wraps them internally via toFiberHandler/collectHandlers. The github.com/gofiber/fiber/v3/middleware/adaptor package is legacy and should not be suggested for tests or code using native net/http handler support.

Applied to files:

  • client/response.go
🧬 Code graph analysis (2)
client/core.go (1)
client/response.go (1)
  • ReleaseResponse (232-235)
client/response_test.go (2)
client/client.go (2)
  • Get (831-833)
  • New (752-757)
client/response.go (3)
  • Response (19-25)
  • AcquireResponse (222-228)
  • ReleaseResponse (232-235)
🔇 Additional comments (13)
client/client.go (1)

529-541: LGTM! Clean delegation to transport layer.

The methods correctly delegate streaming configuration to the transport interface, which handles the underlying fasthttp client(s) appropriately. The implementation is thread-safe since the transport layer manages synchronization internally.

client/core.go (3)

89-93: LGTM! Nil check prevents double-free.

The conditional release correctly guards against releasing a nil response, which can happen when respv is reassigned to nil after swapping into resp.RawResponse (line 127).


96-98: LGTM! BodyStream forwarding preserves streaming semantics.

Correctly propagates the request's BodyStream to the fasthttp request, maintaining the original ContentLength. This enables streaming request bodies alongside the new streaming response body support.


125-130: LGTM! RawResponse swap prevents resource leaks.

The swap sequence is correct:

  1. Save original RawResponse reference
  2. Assign the new respv to resp.RawResponse
  3. Nil out respv (preventing double-free in defer)
  4. Release the original RawResponse if it existed

This ensures clean resource management when responses are reused or when RawResponse is replaced.

client/response.go (1)

155-179: LGTM! Save() correctly handles both streaming and non-streaming paths.

The conditional branching on IsStreaming() ensures efficient streaming when enabled while maintaining backward compatibility for buffered responses. Using io.Copy is appropriate for both cases.

client/response_test.go (4)

546-654: LGTM! Comprehensive streaming Save() tests with proper error handling.

The new tests correctly exercise:

  • Streaming to io.Writer without closing (verifying Save doesn't close user-provided writers)
  • Write errors during streaming (using the testErrorWriter helper)

The test helpers (testClosableBuffer, testErrorWriter) are well-designed for their purpose.


656-1083: Excellent streaming test coverage with true server-side streaming.

The tests now properly use SendStreamWriter and SendStream as requested in past reviews, ensuring the server actually streams data progressively. Coverage includes:

  • Basic streaming and large responses
  • Chunked streaming with delays
  • Server-sent events
  • Progressive JSON streaming
  • Connection interruption handling
  • Large response validation
  • Stream identity verification

All scenarios exercise real streaming behavior rather than masking it with client-side buffering.


1085-1244: LGTM! Thorough fallback and state consistency tests.

The fallback tests verify:

  • Non-streaming responses correctly use bytes.Reader fallback
  • Empty responses are handled correctly
  • Large non-streaming responses work via fallback path
  • BodyStream() works regardless of streaming state
  • IsStreaming() accurately reflects the streaming mode

This ensures the API is robust for both streaming and non-streaming use cases.


1246-1373: LGTM! Complete Save() streaming validation.

Additional tests cover:

  • File-path streaming (end-to-end)
  • File write errors (permission/path issues)
  • Acquire/Release pooling correctness
  • Large streaming writes with error injection

The coverage is comprehensive and exercises real-world failure scenarios.

client/transport.go (4)

30-31: LGTM! Clean interface extension for streaming support.

The new methods integrate naturally with the existing httpClientTransport interface, maintaining consistency with the getter/setter pattern used for TLSConfig and Dial.


80-86: LGTM! Straightforward delegation to fasthttp.Client.

The implementation correctly proxies to the underlying fasthttp.Client's StreamResponseBody field for both getter and setter.


134-140: LGTM! Straightforward delegation to fasthttp.HostClient.

The implementation correctly proxies to the underlying fasthttp.HostClient's StreamResponseBody field for both getter and setter.


200-221: LGTM! Correct traversal logic for nested load balancers.

The implementation properly handles the LBClient's nested structure:

  • Getter: Returns false if no clients exist (safe default), then walks the first client tree to find the StreamResponseBody setting
  • Setter: Uses forEachHostClient to propagate the setting to all nested HostClient instances

This ensures consistent behavior across nested and flat load-balancing configurations.

@Abhirup-99
Copy link
Author

fixed the lint errors

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 0

🧹 Nitpick comments (4)
ctx_test.go (1)

3778-3778: Verify timeout necessity for file save operations.

The 5-second timeout (5x the default) added to this file save test suggests potential performance concerns or CI environment slowness. While reasonable for file I/O, ensure this isn't masking an underlying issue with file save performance.

Consider:

  • Is this timeout consistently needed, or only in specific CI environments?
  • Should other file operation tests (Test_Ctx_FormFile at line 1806, various Test_Ctx_SendFile* tests) also have explicit timeouts for consistency?
docs/client/rest.md (1)

409-423: Consider enhancing streaming documentation with usage examples and caveats.

The current documentation provides basic information but could benefit from additional context to help users avoid common pitfalls:

  • Default behavior (streaming disabled by default)
  • How to override at request level via Request.SetStreamResponseBody(bool)
  • Usage caution: when streaming is enabled, Body() may return empty; prefer BodyStream() for consumption
  • A minimal example showing how to consume the stream (e.g., io.Copy(file, resp.BodyStream()))
  • Note about checking Response.IsStreaming() to verify streaming state
client/response.go (2)

92-101: Consider clarifying the documentation regarding Body() behavior when streaming.

The note mentions that Body() may return an empty slice after using BodyStream(), but it could be more explicit about when this occurs (specifically, when the underlying fasthttp response is in streaming mode and the stream has been consumed).

Consider this documentation refinement:

-// BodyStream returns the response body as a stream reader.
-// Note: When using BodyStream(), the response body is not copied to memory,
-// so calling Body() afterwards may return an empty slice.
+// BodyStream returns an io.Reader for the response body.
+// If streaming is enabled (via Client.SetStreamResponseBody or Request.SetStreamResponseBody),
+// this returns the underlying stream reader without buffering.
+// Otherwise, it returns a bytes.Reader wrapping the buffered body.
+//
+// Note: When streaming is enabled and the stream is consumed, Body() will return an empty slice.
+// Check IsStreaming() to determine the current mode.

155-179: Consider consolidating the streaming/non-streaming branches in Save().

Both the file path and io.Writer cases use identical branching logic (if r.IsStreaming() ... else ...). Since BodyStream() already handles both streaming and non-streaming cases by returning the appropriate reader, the conditional branches could be simplified.

Apply this diff to simplify and reduce duplication:

 		// Create and write to file
 		outFile, err := os.Create(file)
 		if err != nil {
 			return fmt.Errorf("failed to create file: %w", err)
 		}
 		defer func() { _ = outFile.Close() }() //nolint:errcheck // not needed
 
-		if r.IsStreaming() {
-			_, err = io.Copy(outFile, r.BodyStream())
-		} else {
-			_, err = io.Copy(outFile, bytes.NewReader(r.Body()))
-		}
+		_, err = io.Copy(outFile, r.BodyStream())
 
 		if err != nil {
 			return fmt.Errorf("failed to write response body to file: %w", err)
 		}
 
 		return nil
 
 	case io.Writer:
-		var err error
-		if r.IsStreaming() {
-			_, err = io.Copy(p, r.BodyStream())
-		} else {
-			_, err = io.Copy(p, bytes.NewReader(r.Body()))
-		}
+		_, err := io.Copy(p, r.BodyStream())
 
 		if err != nil {
 			return fmt.Errorf("failed to write response body to writer: %w", err)
 		}
 
 		return nil

This simplification is safe because BodyStream() already provides the correct reader for both modes.

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 402be56 and 3c9c80c.

📒 Files selected for processing (10)
  • client/client.go (1 hunks)
  • client/client_test.go (1 hunks)
  • client/core.go (2 hunks)
  • client/core_test.go (1 hunks)
  • client/response.go (3 hunks)
  • client/response_test.go (2 hunks)
  • client/transport.go (4 hunks)
  • client/transport_test.go (1 hunks)
  • ctx_test.go (2 hunks)
  • docs/client/rest.md (1 hunks)
✅ Files skipped from review due to trivial changes (1)
  • client/client_test.go
🚧 Files skipped from review as they are similar to previous changes (3)
  • client/core.go
  • client/transport.go
  • client/client.go
🧰 Additional context used
📓 Path-based instructions (2)
**/*.go

📄 CodeRabbit inference engine (AGENTS.md)

**/*.go: Apply formatting using gofumpt (Make target: format)
Optimize struct field alignment using betteralign (Make target: betteralign)
Modernize code using gopls modernize (Make target: modernize)

Files:

  • client/core_test.go
  • client/response.go
  • client/response_test.go
  • client/transport_test.go
  • ctx_test.go
docs/**

📄 CodeRabbit inference engine (.github/copilot-instructions.md)

Review and update the contents of the docs folder if necessary when modifying code

Files:

  • docs/client/rest.md
🧠 Learnings (12)
📚 Learning: 2025-10-16T07:19:52.418Z
Learnt from: grivera64
Repo: gofiber/fiber PR: 3807
File: adapter_test.go:118-144
Timestamp: 2025-10-16T07:19:52.418Z
Learning: In the Fiber codebase, the linter does not allow `require` assertions from within HTTP handlers (including net/http-style handlers). Use `t.Fatalf`, `t.Errorf`, or similar `testing.T` methods for error handling inside handler functions instead.

Applied to files:

  • client/response.go
  • client/response_test.go
📚 Learning: 2025-09-28T17:39:26.644Z
Learnt from: arturmelanchyk
Repo: gofiber/fiber PR: 3768
File: client/request.go:162-166
Timestamp: 2025-09-28T17:39:26.644Z
Learning: In fasthttp-based code like gofiber, using utils.UnsafeString(key) instead of string(key) is acceptable when values are used immediately within the same call stack and not stored for later use, as it avoids allocations and aligns with fasthttp's zero-allocation design philosophy.

Applied to files:

  • client/response.go
📚 Learning: 2025-10-16T07:15:26.529Z
Learnt from: grivera64
Repo: gofiber/fiber PR: 3807
File: adapter_test.go:118-144
Timestamp: 2025-10-16T07:15:26.529Z
Learning: In Fiber v3, net/http handlers (http.Handler, http.HandlerFunc, or raw func(http.ResponseWriter, *http.Request)) can be passed directly to routing methods like app.Get(), app.Post(), etc. The framework automatically detects and wraps them internally via toFiberHandler/collectHandlers. The github.com/gofiber/fiber/v3/middleware/adaptor package is legacy and should not be suggested for tests or code using native net/http handler support.

Applied to files:

  • client/response.go
📚 Learning: 2024-11-29T12:37:27.581Z
Learnt from: efectn
Repo: gofiber/fiber PR: 3162
File: app_test.go:893-895
Timestamp: 2024-11-29T12:37:27.581Z
Learning: In the `Test_App_ShutdownWithContext` function in `app_test.go`, the `clientDone` channel is used to synchronize the client's request completion before proceeding, eliminating the need for additional `time.Sleep` calls.

Applied to files:

  • client/response_test.go
  • ctx_test.go
📚 Learning: 2024-09-25T16:18:34.719Z
Learnt from: sixcolors
Repo: gofiber/fiber PR: 3016
File: middleware/session/config.go:122-122
Timestamp: 2024-09-25T16:18:34.719Z
Learning: In `DefaultErrorHandler(c *fiber.Ctx, err error)`, since `c` is a pointer to an interface, we need to dereference `*c` when calling interface methods like `SendStatus`.

Applied to files:

  • client/response_test.go
📚 Learning: 2024-10-02T23:03:31.727Z
Learnt from: sixcolors
Repo: gofiber/fiber PR: 3016
File: middleware/session/store.go:164-167
Timestamp: 2024-10-02T23:03:31.727Z
Learning: Unit tests in this project use testify require.

Applied to files:

  • client/response_test.go
📚 Learning: 2024-10-08T19:06:06.583Z
Learnt from: sixcolors
Repo: gofiber/fiber PR: 2922
File: middleware/cors/utils.go:63-71
Timestamp: 2024-10-08T19:06:06.583Z
Learning: The project uses the testify/assert package for assertions in unit tests.

Applied to files:

  • client/response_test.go
📚 Learning: 2024-11-15T07:56:21.623Z
Learnt from: ReneWerner87
Repo: gofiber/fiber PR: 3161
File: app.go:923-932
Timestamp: 2024-11-15T07:56:21.623Z
Learning: In the Fiber framework, breaking changes are acceptable when moving from version 2 to version 3, including modifications to method signatures such as in the `Test` method in `app.go`.

Applied to files:

  • ctx_test.go
📚 Learning: 2024-11-08T04:10:42.990Z
Learnt from: gaby
Repo: gofiber/fiber PR: 3193
File: middleware/cache/cache_test.go:897-897
Timestamp: 2024-11-08T04:10:42.990Z
Learning: In the Fiber framework, `Context()` is being renamed to `RequestCtx()`, and `UserContext()` to `Context()` to improve clarity and align with Go's context conventions.

Applied to files:

  • ctx_test.go
📚 Learning: 2024-10-12T10:01:44.206Z
Learnt from: sixcolors
Repo: gofiber/fiber PR: 3016
File: middleware/session/middleware_test.go:190-191
Timestamp: 2024-10-12T10:01:44.206Z
Learning: When testing session `IdleTimeout` expiration, it's acceptable to use `time.Sleep` to simulate the passage of time in tests.

Applied to files:

  • ctx_test.go
📚 Learning: 2024-10-08T19:06:06.583Z
Learnt from: sixcolors
Repo: gofiber/fiber PR: 3051
File: middleware/session/session.go:215-216
Timestamp: 2024-10-08T19:06:06.583Z
Learning: Parallel tests for `Session.Save` already exist in the `middleware/session/session_test.go` file, specifically in the `Test_Session_Save` and `Test_Session_Save_Expiration` functions.

Applied to files:

  • ctx_test.go
📚 Learning: 2024-11-10T23:44:13.704Z
Learnt from: gaby
Repo: gofiber/fiber PR: 3193
File: middleware/adaptor/adaptor.go:111-111
Timestamp: 2024-11-10T23:44:13.704Z
Learning: In the `middleware/adaptor/adaptor.go` file of the Fiber framework, when updating context handling, replacing `c.Context()` with `c.RequestCtx()` is appropriate to access the `fasthttp.RequestCtx`.

Applied to files:

  • ctx_test.go
🧬 Code graph analysis (2)
client/response_test.go (2)
client/client.go (2)
  • Get (831-833)
  • New (752-757)
client/response.go (3)
  • Response (19-25)
  • AcquireResponse (222-228)
  • ReleaseResponse (232-235)
ctx_test.go (1)
app.go (1)
  • TestConfig (1090-1101)
🔇 Additional comments (10)
ctx_test.go (1)

3820-3820: Consistent timeout for memory storage test.

This test uses in-memory storage yet requires the same 5-second timeout as the disk-based Test_Ctx_SaveFile. This suggests the timeout is needed for multipart upload processing rather than the storage backend itself, which is reasonable.

For consistency, verify that all file upload/save tests have appropriate timeouts to prevent test flakiness in CI environments.

client/core_test.go (1)

386-391: LGTM!

The streaming method stubs correctly satisfy the transport interface for this test helper. The no-op implementation is appropriate since blockingErrTransport is designed to simulate errors, not actual streaming behavior.

client/response.go (1)

103-106: LGTM!

The IsStreaming() implementation correctly identifies when the response is in streaming mode by checking for a non-nil BodyStream().

client/transport_test.go (1)

350-548: LGTM! Comprehensive streaming state test coverage.

The tests thoroughly validate streaming state propagation across all transport implementations:

  • StandardClientTransport: verifies default, enable, and disable behavior with fasthttp.Client state checks
  • HostClientTransport: validates streaming toggles on HostClient
  • LBClientTransport: covers empty, single, multiple, nested, and mixed client configurations
  • Interface tests: ensure consistent behavior across all transport types

The nested LB client scenarios are particularly valuable for catching propagation bugs in complex load balancing setups.

client/response_test.go (6)

546-620: LGTM! Well-designed Save() streaming tests with proper error handling.

The new streaming-specific Save() tests effectively validate:

  1. Writer closing behavior: correctly verifies that Save() does not close the provided writer (lines 546-585)
  2. Write error handling: uses a custom testErrorWriter to simulate mid-stream write failures and verify proper error propagation (lines 587-619)

Both tests use SendStreamWriter to ensure genuine streaming behavior, addressing previous review feedback.


622-654: LGTM! Clean test helper implementations.

The test helper types are well-designed for their specific purposes:

  • testClosableBuffer: tracks Close() calls to verify Save() doesn't close provided writers
  • testErrorWriter: simulates write failures after a threshold to test error handling in streaming paths

Both helpers are appropriately scoped (non-exported) and implement only the necessary interfaces.


656-1083: Excellent comprehensive streaming test coverage.

The Test_Response_BodyStream test suite thoroughly validates streaming behavior across diverse scenarios:

  1. Basic streaming (lines 659-679): verifies simple stream consumption
  2. Large response streaming (lines 681-718): validates progressive reads with buffer management
  3. Compare with regular body (lines 720-751): ensures streaming and non-streaming produce identical results
  4. Chunked streaming with delays (lines 753-804): tests incremental data arrival
  5. Server-sent events (lines 806-858): validates SSE streaming pattern
  6. Progressive JSON streaming (lines 860-909): tests streaming JSON arrays
  7. Connection interruption (lines 911-972): validates behavior when stream is closed mid-read
  8. Large response validation (lines 974-1040): tests multi-chunk streaming with pattern verification
  9. Stream object identity (lines 1042-1082): verifies correct reader type based on streaming state

All subtests properly use SendStreamWriter or SendStream to ensure genuine streaming behavior, addressing past review concerns.


1085-1169: LGTM! Comprehensive fallback behavior tests.

The Test_Response_BodyStream_Fallback suite effectively validates non-streaming scenarios:

  1. Non-streaming fallback (lines 1087-1107): verifies bytes.Reader fallback when streaming is disabled
  2. Empty response (lines 1109-1129): tests edge case of empty body
  3. Large non-streaming response (lines 1131-1168): confirms fallback works correctly even with large payloads

These tests ensure BodyStream() provides a consistent API regardless of streaming mode.


1171-1244: LGTM! IsStreaming() behavior properly validated.

The Test_Response_IsStreaming suite covers:

  1. Streaming enabled with large response (lines 1174-1196)
  2. Streaming disabled (lines 1198-1212)
  3. BodyStream() works regardless of streaming state (lines 1214-1243)

The third subtest is particularly valuable as it confirms the API remains consistent whether streaming is enabled or not.


1246-1373: LGTM! Additional Save() test coverage for edge cases.

The remaining Save() tests validate:

  1. Streaming to file path (lines 1246-1293): confirms Save() works correctly when streaming to a file path
  2. File error handling (lines 1295-1315): validates error handling when file creation fails
  3. Acquire/Release pool behavior (lines 1317-1338): verifies response pooling works correctly
  4. File write errors (lines 1340-1373): tests error handling when writing to /dev/full fails

These tests provide good coverage of error paths and edge cases.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

Status: In Progress

Development

Successfully merging this pull request may close these issues.

📝 [Proposal]: Add support for fasthttp.StreamResponseBody

6 participants